On Sat, Mar 21, 2020 at 02:35:47PM -0400, Martin Becze wrote:
A few things got stale and need to be merged so I have regenerated the patch
set! Let me know if there is anymore things to do.
Alright, I started taking a close look at the patches and they need some
more work. At least, the commit messages need to be completed. The
importer does work, which is amazing, so we are almost there :)
In general, the commit messages need to be rewritten to match our style.
This means they should use complete English sentences with standard
capitalization and punctuation. I'm happy to help if necessary. English
is my native language.
We also should try to match previous commit messages that touch the same
code, because they are good examples. Take this example, the first patch:
Subject: [PATCH v11 1/9] guix: import: (recursive-import) Allow for version
numbers
This commit title should be written like this:
import: utils: 'recursive-import' accepts an optional version parameter.
I learned that by doing `git log guix/import/utils.scm` and reading the
previous commits.
* guix/import/utils.scm (package->definition): added optional `append-version?`
* guix/import/utils.scm (recursive-import): added key `version` and
moved `repo` to be a key
When changing multiple variables in the same file, the filename can be
mentioned only once. I would write that like this:
* guix/import/utils.scm (recursive-import): Add the VERSION key. Make REPO a
key.
(package->definition): Accept an optional 'append-version?' key.
I began to rewrite the rest of the commit message like this:
* guix/import/cran.scm (cran->guix-package): Change the REPO parameter to a key.
(cran-recursive-import): Likewise.
* guix/import/elpa.scm (elpa->guix-package): Likewise.
(elpa-recursive-import): Likewise.
* guix/import/gem.scm (gem-recursive-import): Likewise.
* guix/scripts/import/cran.scm (guix-import-cran): Likewise.
* guix/scripts/import/elpa.scm (guix-import-elpa): Likewise.
* guix/import/opam.scm (opam-recursive-import): Likewise.
* guix/import/pypi.scm (pypi-recursive-import): Likewise.
* guix/import/stackage.scm (stackage-recursive-import): Likewise.
However, I found some issues.
* guix/import/gem.scm (gem->guix-package): change `repo` to a key
This change does not exist in this commit.
tests/elpa.scm | 3 +-
tests/import-utils.scm | 8 +++--
And these changes are not mentioned in the commit message.
These issues make it hard to review the patches effectively.
What do you think? Can you make these changes?