unofficial mirror of guix-devel@gnu.org 
 help / color / mirror / code / Atom feed
From: Liliana Marie Prikler <liliana.prikler@gmail.com>
To: Maxime Devos <maximedevos@telenet.be>, guix-devel@gnu.org
Subject: Re: New review checklist
Date: Fri, 01 Apr 2022 20:25:37 +0200	[thread overview]
Message-ID: <01ff99a0452cb12718938646388302ac372a7416.camel@gmail.com> (raw)
In-Reply-To: <447d4df82b8626181b37bedb37ba60d212cd98e9.camel@telenet.be>

Hi Maxime,

Am Freitag, dem 01.04.2022 um 19:46 +0200 schrieb Maxime Devos:
> [...]
> I know there have been some discussions in the past about whether
> git-version should be used when a commit is explicitly chosen,
> whether
> tags should be used instead of commits, how high a risk there is that
> version->commit can become multi-valued, ‘tricking peer review’ ...
> 
> However, my question isn't about any of that.  It is only about the
> let-binding itself, in situations where the bound variable is only
> used in a single place.  What is the reason for doing
> 
> (let ((commit "cabba9e..."))
>   (package
>     (name "foobar")
>     (version "0.1.2")
>     (source (origin ...
>               ;; this is the only use of the 'commit' variable bound
> in
>               ;; the above 'commit'
>               (commit commit)))
>     ...))
> 
> when it can be simplified to
> 
> (package
>   (name "foobar")
>   (version "0.1.2")
>   (source (origin ... (commit "cabba9e..."))))?
> 
> I mean, we don't do this for, say, 'name', 'version' and 'uri':
> 
> ;; these three variables are only used in a single location
> (let ((name "foobar")
>       (version "0.1.2")
>       (uri "https://foo.bar"))
>   (package
>     (name name)
>     (version version)
>     (source (origin (uri uri) (commit <some-reference>) [...]))
>     ...))
> 
> Why would things be different for 'commit' here?  How does putting
> the value of 'commit' in a let-form reduce surprises?
The main goal of let-binding commit and revision is to allow for easier
change.  Suppose you need to reference some half-release for some
obscure reason, then this style makes it easier to switch to what is
already established praxis.

In general, consider the poor soul who may have to read and maintain
your code after you get hit by a car because neither busses nor trams
run in your region.


  reply	other threads:[~2022-04-01 18:27 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-01  4:14 New review checklist Liliana Marie Prikler
2022-04-01  6:30 ` Maxime Devos
2022-04-01 17:03   ` Liliana Marie Prikler
2022-04-01 17:46     ` Maxime Devos
2022-04-01 18:25       ` Liliana Marie Prikler [this message]
2022-04-02 13:38         ` Bengt Richter
2022-04-02 14:45           ` Liliana Marie Prikler
2022-04-01 18:05   ` Tobias Geerinckx-Rice
2022-04-01  6:56 ` tanguy
2022-04-01  8:31 ` Jonathan McHugh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://guix.gnu.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=01ff99a0452cb12718938646388302ac372a7416.camel@gmail.com \
    --to=liliana.prikler@gmail.com \
    --cc=guix-devel@gnu.org \
    --cc=maximedevos@telenet.be \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).