Package: guix-patches;
Reported by: Philip McGrath <philip <at> philipmcgrath.com>
Date: Thu, 9 Nov 2023 16:09:02 UTC
Severity: normal
Tags: patch
Message #59 received at 67019 <at> debbugs.gnu.org (full text, mbox):
From: Philip McGrath <philip <at> philipmcgrath.com> To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>, 67019 <at> debbugs.gnu.org Subject: Re: [PATCH 03/16] gnu: Add lessc. Date: Wed, 15 Nov 2023 14:35:31 -0500
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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.