|
From: | Martin Becze |
Subject: | [bug#38408] [PATCH v9 3/8] Added Guile-Semver as a dependency to guix |
Date: | Mon, 23 Mar 2020 12:28:04 -0400 |
On 3/23/20 5:50 AM, Martin Becze wrote:
Thanks for the feedback Leo! I will work on this today. On 3/22/20 4:10 PM, Leo Famulari wrote: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 patchset! 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 versionnumbersThis 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 keyWhen 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 keyThis 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?
v12-0008-guix-self-Adds-guile-semver-as-a-depenedency.patch
Description: Text Data
v12-0007-gnu-Add-guile3.0-semver.patch
Description: Text Data
v12-0006-import-crate-Parametrized-importing-of-dev-depen.patch
Description: Text Data
v12-0005-import-utils-Trim-patch-version-from-names.patch
Description: Text Data
v12-0004-import-crate-Memorize-crate-guix-package.patch
Description: Text Data
v12-0003-import-crate-Deduplicate-dependencies.patch
Description: Text Data
v12-0002-import-crate-Use-guile-semver-to-resovle-module-.patch
Description: Text Data
v12-0001-import-utils-recursive-import-accepts-an-optiona.patch
Description: Text Data
[Prev in Thread] | Current Thread | [Next in Thread] |