From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:470:142:3::10]:56489) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jGJjb-0002ZC-E1 for guix-patches@gnu.org; Mon, 23 Mar 2020 05:51:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1jGJja-0002VG-4E for guix-patches@gnu.org; Mon, 23 Mar 2020 05:51:03 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:45500) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1jGJja-0002VA-0A for guix-patches@gnu.org; Mon, 23 Mar 2020 05:51:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1jGJjZ-0001MC-Uu for guix-patches@gnu.org; Mon, 23 Mar 2020 05:51:01 -0400 Subject: [bug#38408] [PATCH v9 3/8] Added Guile-Semver as a dependency to guix Resent-Message-ID: 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> <20200322201018.GB16716@jasmine.lan> From: Martin Becze Message-ID: <45972138-5bf8-821a-178a-c04e6446e076@riseup.net> Date: Mon, 23 Mar 2020 05:50:53 -0400 MIME-Version: 1.0 In-Reply-To: <20200322201018.GB16716@jasmine.lan> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Leo Famulari Cc: 38408@debbugs.gnu.org, Ludovic =?UTF-8?Q?Court=C3=A8s?= , Efraim Flashner , jsoo1@asu.edu 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 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? >