Package: guix-patches;
Reported by: Philip McGrath <philip <at> philipmcgrath.com>
Date: Thu, 9 Nov 2023 16:09:02 UTC
Severity: normal
Tags: patch
Message #74 received at 67019 <at> debbugs.gnu.org (full text, mbox):
From: Liliana Marie Prikler <liliana.prikler <at> gmail.com> To: Philip McGrath <philip <at> philipmcgrath.com>, 67019 <at> debbugs.gnu.org Subject: Re: [PATCH 03/16] gnu: Add lessc. Date: Thu, 16 Nov 2023 08:18:27 +0100
Hi,
Am Mittwoch, dem 15.11.2023 um 20:51 -0500 schrieb Philip McGrath:
> Hi,
>
> On 11/15/23 20:17, Liliana Marie Prikler wrote:
> > Hi,
> >
> > Am Mittwoch, dem 15.11.2023 um 19:03 -0500 schrieb Philip McGrath:
> > > Hi,
> > >
> > > On 11/15/23 15:23, Liliana Marie Prikler wrote:
> > > > Am Mittwoch, dem 15.11.2023 um 14:35 -0500 schrieb Philip
> > > > McGrath:
> > > > > [...]
> > > > >
> > > > > To clarify, do you mean vertical or horizontal?
> > > > I do mean horizontal.
> > > >
> > > > [...]
> > > >
> > > > >
> > > > > I could also imagine breaking these lines:
> > > > >
> > > > > >> + (("scripts" @ . alist)
> > > > > >> + `("scripts" @ ,@(map
> > > > > (match-
> > > > > lambda
> > > > >
> > > > > instead as:
> > > > >
> > > > > >> + (("scripts"
> > > > > >> + @ . alist)
> > > > > >> + `("scripts"
> > > > > >> + @ ,@(map (match-lambda
> > > > >
> > > > > but that doesn't seem like much of an improvement to me.
> > > > But what about breaking lines before (match-lambda? That ought
> > > > to
> > > > at least give us enough to get (_ #f) onto a single line, no?
> > > >
> > >
> > > Maybe I'm confused: there isn't (_ #f) anywhere.
> > There was a (_ #t) in the filter, though :)
> > In any case, such trivial matches should fit onto one line imho.
> >
> > > There is currently enough space to put (other other) on a single
> > > line, but I thought it was better style to put a newline between
> > > the
> > > match pattern and the expression, especially when the pattern is
> > > not
> > > _.
> > IMHO, this only makes sense for non-trivial replacements. If you
> > keep
> > some piece of data as-is, you more often than not don't want to
> > draw
> > attention to this implementation detail by blowing up its size.
> >
>
> I don't think the content of the right-hand side is relevant: in my
> view, the purpose of the newline is to make the shape of the clause
> clear, especially given that the left-hand side is not an expression.
> The fact that Guix's style forbids square brackets makes the newline
> even more necessary, IMO.
>
> In any case, what arrangement of whitespace in patch-build-script
> would unblock this patch series for you?
If push comes to shove, I can rearrange the whitespace as I want, given
my committer privileges; it's just that you've complained about me
abusing them before, so I don't really want to.
Anyhow, stuff like (_ #t), (_ #f), (x x), … should all fit onto a
single line.
> > >
> > > Using delete in do-not-target-es5 does seem like a minor
> > > improvement:
> > >
> > > > (add-after 'avoid-parse-node-version 'do-not-target-
> > > > es5
> > > > (lambda args
> > > > ;; esbuild can't compile all features to ES5
> > > > (with-atomic-json-file-replacement
> > > > "tsconfig.json"
> > > > (match-lambda
> > > > (('@ . alist)
> > > > (cons '@
> > > > (map (match-lambda
> > > > (("compilerOptions" '@ . alist)
> > > > `("scripts" @ ,@(delete '("target"
> > > > "ES5")
> > > > alist)))
> > > > (other
> > > > other))
> > > > alist)))))))
> > Fun fact, you could inline this delete into the "compilerOptions"
> > line or sexp at least, by using (= (cute delete '("target" "ES5")
> > <>) options). That being said, it's not necessary to do so; the
> > delete you currently have works fine.
> >
> > Anyhow, since this isn't a clean alist, I'd use a different
> > variable name, perhaps "options"?
>
> All of the variables named alist are, in fact, alists.
TIL.
> I noticed, though, that this was inadvertently renaming
> "compilerOptions" to "scripts" and thus effectively patching out all
> the other options, too: things seemed to work regardless, but here's
> a correct version:
>
> > (add-after 'avoid-parse-node-version 'do-not-target-es5
> > (lambda args
> > ;; esbuild can't compile all features to ES5
> > (with-atomic-json-file-replacement "tsconfig.json"
> > (match-lambda
> > (('@ . alist)
> > (cons '@
> > (map (match-lambda
> > (("compilerOptions" '@ . alist)
> > `("compilerOptions" @ ,@(delete
> > '("target" . "ES5")
> >
> > alist)))
> > (other
> > other))
> > alist)))))))
Now that looks like a proper alist to me :)
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.