GNU bug report logs - #67019
[PATCH 00/16] gnu: Add KaTeX, lessc, and flow-remove-types.

Previous Next

Full log

View this message in rfc822 format

From: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
To: Philip McGrath <philip <at> philipmcgrath.com>, 67019 <at> debbugs.gnu.org
Subject: [bug#67019] [PATCH 03/16] gnu: Add lessc.
Date: Thu, 16 Nov 2023 02:17:38 +0100

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.