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

[Philip McGrath <philip@philipmcgrath.com>]

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?

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.

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.)

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.


>> +    (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?

Philip