Hi Pierre,
thank you for reviewing!
On 06.01.19 20:00, Pierre Neidhardt wrote:
> 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.

As the new function can be defined with a normal lambda and not a
lambda* I just used define-public.

>> +;;; 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---

Done.

>> +;;; 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.

Done.

>> +          srcpkg pkgname src-file
> 
> Prefer complete words over abbreviations.  Here I'd suggest
> 
>   source-package
>   name
>   source-file

Done. name is called package-name.

>> +      (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---

Obsolete as this is now moved again to the final package definition.
Thanks for the tip :) I'm still quite new to scheme.

> 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---

I was also thinking about this. But with stuffing everything into the
function to evaluate to the final definition made multiple files
difficult as it would complicate the data structure for substitutions.
As this is not part of the function this is not a problem anymore.

Maybe we could make the function even more generic if we would just let
it modify the origin object.

> 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.

Done. Patches are attached.

Tim.