GNU bug report logs - #33598
Optimizations for emacs-clang-format and emacs-clang-rename

Previous Next

Full log

Message #38 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Pierre Neidhardt <mail <at> ambrevar.xyz>
To: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Sun, 06 Jan 2019 20:00:19 +0100

[Message part 1 (text/plain, inline)]

Hi Tim,

I just reviewed your patch. Looks pretty good overall, thanks!

A few things:

> +(export package-elisp-from-package)

This should be placed at the beginning of the file in the (define-module...
See bootstrap.scm.

> +;;; Returns a package definition that packages an emacs-lisp file from the
> +;;; SRCPKG source. The package has the name PKGNAME and packages the file
> +;;; SRC-FILE from the source in its root directory as TARGET-FILE or the
> +;;; basename of SRC-FILE where INPUTS NATIVE-INPUTS and PROPAGATED-INPUTS are
> +;;; added as package inputs and SUBSTITUTIONS substitutions will be performed
> +;;; on the elisp file and SYNOPSIS and DESCRIPTION as the package synopsis and
> +;;; description.
> +(define* (package-elisp-from-package

Move the ";;;" comment to a docstring, e.g.

--8<---------------cut here---------------start------------->8---
(define* (package-elisp-from-package
          ...)
  "Return ..."
--8<---------------cut here---------------end--------------->8---

> +;;; Returns a package definition that packages an emacs-lisp file from the

"Return", not "Returns".

> +;;; SRCPKG source. The package has the name PKGNAME and packages the file

Separate sentences with two spaces.

> +          srcpkg pkgname src-file

Prefer complete words over abbreviations.  Here I'd suggest

  source-package
  name
  source-file

> +      (synopsis (if synopsis
> +                    synopsis
> +                    (package-synopsis srcpkg)))
> +      (description (if description
> +                       description
> +                       (package-description srcpkg))))))

A more Lispy way:

--8<---------------cut here---------------start------------->8---
 +      (synopsis (or synopsis
 +                    (package-synopsis srcpkg)))
 +      (description (or description
 +                       (package-description srcpkg))))))
--8<---------------cut here---------------end--------------->8---

Regarding the function parameters, I would turn SOURCE-FILE into SOURCE-FILES to
make it more generic.  Indeed, the Emacs library could very well be split over
multiple files.

One thing I'm not too sure about is the replication of the structure fields as
keys.
I think it's easier to ignore those and let the user define them as follows:

--8<---------------cut here---------------start------------->8---
(define-public emacs-clang-rename
  (package
    (inherit (package-elisp-from-package
             clang
             "emacs-clang-rename"
             "tools/clang-rename/clang-rename.el"))
    (arguments ...)))
--8<---------------cut here---------------end--------------->8---

Makes sense?  This would also be more robust in case the package structure
changes someday.

Finally, rebase your changes so that you directly use the last function, no
need for the clang-specific function to appear in the history of commits.

Thank you again for the good work!

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.