guix-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Philip McGrath
Subject: [bug#67019] [PATCH 03/16] gnu: Add lessc.
Date: Wed, 15 Nov 2023 20:51:49 -0500
User-agent: Mozilla Thunderbird

Hi,

On 11/15/23 20:17, Liliana Marie Prikler wrote:
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.


I don't think the content of the right-hand side is relevant: in my view, the purpose of the newline is to make the shape of the clause clear, especially given that the left-hand side is not an expression. The fact that Guix's style forbids square brackets makes the newline even more necessary, IMO.

In any case, what arrangement of whitespace in patch-build-script would unblock this patch series for you?


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"?


All of the variables named alist are, in fact, alists.

I noticed, though, that this was inadvertently renaming "compilerOptions" to "scripts" and thus effectively patching out all the other options, too: things seemed to work regardless, but here's a correct version:

          (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)
                            `("compilerOptions" @ ,@(delete '("target" . "ES5")
                                                            alist)))
                           (other
                            other))
                         alist)))))))

Philip





reply via email to

[Prev in Thread] Current Thread [Next in Thread]