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

[Liliana Marie Prikler]

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.

> Breaking before match-lambda gets enough space to put (cons "build" 
> new-build-script) on a single line, but I don't think it looks better
> overall:
> 
> >           (add-after 'do-not-target-es5 'patch-build-script
> >             (lambda args
> >               (define new-build-script
> >                 (string-join
> >                  `("esbuild"
> >                    "--platform=node"
> >                    "--format=cjs"
> >                    "--outdir=lib"
> >                    ,@(find-files "src/less" "\\.js$")
> >                    ,@(find-files "src/less-node" "\\.js$"))))
> >               (with-atomic-json-file-replacement "package.json"
> >                 (match-lambda
> >                   (('@ . alist)
> >                    (cons '@
> >                     (map
> >                      (match-lambda
> >                        (("scripts" @ . alist)
> >                         `("scripts" @ ,@(map
> >                                          (match-lambda
> >                                            (("build" . _)
> >                                             (cons "build" new-
> > build-script))
> >                                            (other
> >                                             other))
> >                                          alist)))
> >                        (other
> >                         other))
> >                      alist)))))))
> 
> 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"?

Cheers