GNU bug report logs - #67019
[PATCH 00/16] gnu: Add KaTeX, lessc, and flow-remove-types.

Previous Next

Full log

View this message in rfc822 format

From: Philip McGrath <philip <at> philipmcgrath.com>
To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>, 67019 <at> debbugs.gnu.org
Subject: [bug#67019] [PATCH 03/16] gnu: Add lessc.
Date: Wed, 15 Nov 2023 20:51:49 -0500

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.