From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:470:142:3::10]:37152) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jG6w3-0006nM-3R for guix-patches@gnu.org; Sun, 22 Mar 2020 16:11:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1jG6w1-0008FC-Vi for guix-patches@gnu.org; Sun, 22 Mar 2020 16:11:03 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:44970) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1jG6w1-0008F4-Rf for guix-patches@gnu.org; Sun, 22 Mar 2020 16:11:01 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1jG6w1-0004DU-NB for guix-patches@gnu.org; Sun, 22 Mar 2020 16:11:01 -0400 Subject: [bug#38408] [PATCH v9 3/8] Added Guile-Semver as a dependency to guix Resent-Message-ID: Date: Sun, 22 Mar 2020 16:10:18 -0400 From: Leo Famulari Message-ID: <20200322201018.GB16716@jasmine.lan> References: <20200217100345.GI1968@E5400> <87wo8l702y.fsf@gnu.org> <874kvld2bl.fsf@gnu.org> <87sgj4nwjw.fsf@gnu.org> <8453e3ce-bc5a-3bb3-1bfb-deaca7ca11d3@riseup.net> <6386cf3e-1156-8d09-a264-cacbb4935f03@riseup.net> <47ed3e36-0d9f-cd6f-a64b-e3f9c9d71537@riseup.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <47ed3e36-0d9f-cd6f-a64b-e3f9c9d71537@riseup.net> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Martin Becze Cc: 38408@debbugs.gnu.org, Ludovic =?UTF-8?Q?Court=C3=A8s?= , Efraim Flashner , jsoo1@asu.edu 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?