Subject: [bug#67019] [PATCH 03/16] gnu: Add lessc.

From: Liliana Marie Prikler <[email-masked]>
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 :)