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

[Liliana Marie Prikler]

Am Mittwoch, dem 15.11.2023 um 14:35 -0500 schrieb Philip McGrath:
> Hi,
> 
> Thanks for taking a look at this!
> 
> On 11/10/23 19:56, Liliana Marie Prikler wrote:
> > Am Donnerstag, dem 09.11.2023 um 11:26 -0500 schrieb Philip
> > McGrath:
> > > * gnu/packages/web.scm (lessc): New variable.
> > > 
> > > [...]
>  >>
> > > +          (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" @ ,@(filter (match-
> > > lambda
> > > +                                                      (("target"
> > > "ES5")
> > > +                                                       #f)
> > > +                                                      (_
> > > +                                                       #t))
> > > +                                                    alist)))
> > > +                           (other
> > > +                            other))
> > > +                         alist)))))))
> > > +          (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)))))))
> > Can we somehow save a bit of horizontal real-estate here?  Same
> > goes
> > for 1 and 2.
> 
> To clarify, do you mean vertical or horizontal?
I do mean horizontal.

> The long list of dependencies to delete does take a *lot* of vertical
> space, especially in this patch. The obvious alternative would be to
> put  more than one dependency on the same line. I'm not opposed to
> that, but it would deviate from normal style and could make future
> diffs less clear.
Speaking of which, can we has regexps here?

> For horizontal space, I don't really like any of the alternatives
> I've thought of. The culprit in each case seems to be the three 
> `match-lambda`s under `with-atomic-json-file-replacement`.
> (Specifically in do-not-target-es5, I guess the innermost one could
> be replaced with just `remove`, which might help a little.)
> 
> In normal programming, I'd want to abstract the three patch-build-
> script phases into a helper function that would take the new-build-
> script string as an argument, but that's a bit awkward to do with
> build-side code in Guix. Putting it in guix/build/node-build-
> system.scm (like 
> delete-dependencies) would trigger a lot of rebuilds that seem hard
> to justify. It could be done as a gexp-producing function, but 
> node-is-what, node-copy-anything, and lessc aren't in the same file:
> I guess the arguments field is delayed, so maybe it wouldn't create a
> cyclic dependency issue, but that seemed to open a whole new can of
> worms.
> 
> (Making e.g. `jsobject-update*` from guix/build/node-build-system.scm
> public would also help, but I have no desire to revisit that.)
I think this is valid criticism of our node-build-system to perhaps
take to another thread.

> Obviously it would be possible, within each G-expression, to lift one
> of the `match-lambda`s (probably the innermost one) to a local
> definition, but IMO that would make the structure of the code less
> obviously correspond to the structure of the JSON being transformed.
> 
> 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?

> > > +    (synopsis "Compiler for @acronym{Less} @acronym{CSS}
> > > language
> > > extension")
> > > +    ;; XXX: @abbr{} seems better for Less (which is always
> > > capitalized that
> > > +    ;; way), but it is rejected as invalid Texinfo markup here.
> > > +    (description "@acronym{Less, Leaner Style Sheets} is a
> > > +backwards-compatible language extension for @acronym{CSS}.  This
> > > package
> > > +provides @command{lessc}, which compiles Less files to plain
> > > @acronym{CSS}.")
> > > +    (license license:asl2.0)))
> > > +
> > IMHO it doesn't make sense to type @acronym without the expansion.
> > 
> 
> I don't have a strong opinion on this, and I only know the tiny
> amount of Texinfo I've picked up for Guix. I inferred from the fact
> that the one-argument version of @acronym{} exists that it should be
> used when applicable. I know that some typographical conventions
> handle capital letters and punctuation in acronyms specially, which
> would be one reason for @acronym{} to exist, but maybe that isn't
> relevant?
Yeah, CAPS aren't relevant here.  The @acronym is typically for giving
meaning to an acronym, but if said meaning has already been given once,
repeating it would not make sense.

Some quotes from the manual:

>    - In Texinfo, an acronym (but not an abbreviation) should consist
>      only of capital letters and periods, no lowercase.
>    [...]
>    - It usually turns out to be quite difficult and/or time-consuming
> to consistently use '@acronym' for all sequences of uppercase
>      letters.  Furthermore, it looks strange for some acronyms to be
>      in the normal font size and others to be smaller.  Thus, a
>      simpler approach you may wish to consider is to avoid '@acronym'
>      and just typeset everything as normal text in all capitals:
>      'GNU', producing the output 'GNU'.
> 
>    - In general, it's not essential to use either of these commands
>      for all abbreviations; use your judgment.  Text is perfectly
>      readable without them.

Cheers