Package: guix-patches;
Reported by: Philip McGrath <philip <at> philipmcgrath.com>
Date: Thu, 9 Nov 2023 16:09:02 UTC
Severity: normal
Tags: patch
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: Wed, 15 Nov 2023 21:23:57 +0100
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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.