Guix style imperfections

  • Open
  • quality assurance status badge
Details
3 participants
  • Liliana Marie Prikler
  • Ludovic Courtès
  • Maxime Devos
Owner
unassigned
Submitted by
Maxime Devos
Severity
normal
M
M
Maxime Devos wrote on 29 Jun 2022 11:33
(address . bug-guix@gnu.org)
9499300db3fe4222f7126240fb2acad3cdf4371b.camel@telenet.be
Hi,

"guix style" occasionally makes some decision that seem a bit
questionable to me. More concretely, copy the definition of guile-
next, put it in a .scm and rename it, and run
"guix style -L . guile-next-styleme". I get:

Toggle quote (6 lines)
> (define-module (test))
> (use-modules (guix packages) (guix git-download) (gnu packages autotools) (gnu packages guile) (guix utils)
> (define-public guile-next
> (let ((version "3.0.7") (revision "0")
> (commit "d70c1dbebf9ac0fd45af4578c23983ec4a7da535"))

Conventionally 'revision' is put on another line -- for these kind of let bindings,
(maybe all?), I would recommend to put all of them on separate lines.

Toggle quote (10 lines)
> (package
> (inherit guile-3.0)
> (name "guile-next-styleme")
> (version (git-version version revision commit))
> (source [snip, LGTM])
> (arguments
> (substitute-keyword-arguments (package-arguments guile-3.0)
> ((#:phases phases
> '%standard-phases) `(modify-phases ,phases

Put %standard-phases on the same line ad #:phases phases and `(modify-phases ,phases
on a new lineg
Toggle quote (8 lines)
> (add-before 'check 'skip-failing-tests
> (lambda _
> (substitute* "test-suite/standalone/test-out-of-memory"
> (("!#") "!#
>
>(exit 77)
>"))

I'd prefer the original "!#\n\n(exit 77)\n" here, but I don't know if that's
something 'Guix style' could feasibly do (there might be situations where a
newline might be appropriate, how could "guix style" which is the case?).

Toggle quote (3 lines)
> (delete-file
> "test-suite/tests/version.test") #t))))))

(Would be nice if "guix style" could be taught to remove those #t, but that seems
more a feature limitation than a bug to me.)

Toggle quote (9 lines)
> (native-inputs (modify-inputs (package-native-inputs guile-3.0)
> (prepend autoconf
> automake
> libtool
> flex
> gnu-gettext
> texinfo
> gperf)))

I'd consider it tidier to put (modify-inputs ...) on a new line

Toggle quote (2 lines)
> (synopsis "Development version of GNU Guile"))))

Question: do people agree with these style choices?

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYrwcURccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7oJFAP4hReQBKh4e79ZjH4zlWDSf0wxF
fIxX69bkVPNSJJIyAQEAlwFXbrhB8Fk0IRrOgnGHf7qfLhintGcnSxfbthuyQgI=
=yODs
-----END PGP SIGNATURE-----


L
L
Liliana Marie Prikler wrote on 29 Jun 2022 12:15
1d570330e9811ec9327ec4f99e2baed4fd922194.camel@ist.tugraz.at
Am Mittwoch, dem 29.06.2022 um 11:33 +0200 schrieb Maxime Devos:
Toggle quote (6 lines)
> Hi,
>
> "guix style" occasionally makes some decision that seem a bit
> questionable to me.  More concretely, copy the definition of guile-
> next, put it in a .scm and rename it, and run
> "guix style -L . guile-next-styleme".  I get:
Before commenting on the individual points, I do think in general guix
style needs to have a "lax" mode and a "strict" mode where the latter
is enabled via "--strict" and keeps certain snippets as-is. All
elements that save vertical space at the cost of horizontal space
should be disabled in strict mode, whereas they might be acceptable in
lax mode.

Toggle quote (10 lines)
> > (define-module (test))
> > (use-modules (guix packages) (guix git-download) (gnu packages
> > autotools) (gnu packages guile) (guix utils)
> > (define-public guile-next
> >  (let ((version "3.0.7") (revision "0")
> >        (commit "d70c1dbebf9ac0fd45af4578c23983ec4a7da535"))
>
> Conventionally 'revision' is put on another line -- for these kind of
> let bindings, (maybe all?), I would recommend to put all of them on
> separate lines.
Agree.

Toggle quote (12 lines)
> >    (package
> >      (inherit guile-3.0)
> >      (name "guile-next-styleme")
> >      (version (git-version version revision commit))
> >      (source [snip, LGTM])
> >      (arguments
> >       (substitute-keyword-arguments (package-arguments guile-3.0)
> >         ((#:phases phases
> >           '%standard-phases) `(modify-phases ,phases
>
> Put %standard-phases on the same line ad #:phases phases and `(modify-
> phases ,phases on a new line
Agree. What's even the point the current style tries to make?

Toggle quote (14 lines)
> >                                 (add-before 'check 'skip-failing-
> > tests
> >                                   (lambda _
> >                                     (substitute* "test-
> > suite/standalone/test-out-of-memory"
> >                                       (("!#") "!#
> >
> > (exit 77)
> > "))
>
> I'd prefer the original "!#\n\n(exit 77)\n" here, but I don't know if
> that's something 'Guix style' could feasibly do (there might be
> situations where a newline might be appropriate, how could "guix style"
> which is the case?).
I'd prefer if strict mode typed those out, but we can keep strings "as-
is" in lax mode, supposing they don't grow beyond the horizontal limit.

Toggle quote (6 lines)
> >                                     (delete-file
> >                                      "test-suite/tests/version.test")
> > #t))))))
>
> (Would be nice if "guix style" could be taught to remove those #t, but
> that seems more a feature limitation than a bug to me.)
It can still do better by not contracting them imho.

Toggle quote (10 lines)
> >      (native-inputs (modify-inputs (package-native-inputs guile-3.0)
> >                       (prepend autoconf
> >                                automake
> >                                libtool
> >                                flex
> >                                gnu-gettext
> >                                texinfo
> >                                gperf)))
>
> I'd consider it tidier to put (modify-inputs ...) on a new line
Here, it depends. I think I'd write this as

(native-inputs 
(modify-inputs (package-native-inputs guile-3.0)
(prepend autoconf automake libtool
flex gperf
gnu-gettext texinfo)))

Toggle quote (3 lines)
> >     (synopsis "Development version of GNU Guile"))))
>
> Question: do people agree with these style choices?
I think some people might actually be okay with a few or even all of
them (juding by how many submit collapsed lets), but I'd like to point
out that they break with Lisp coding guidelines for no good reason.

Regarding the optimization of vertical space, I do think that guix
lacks semantic information to make meaningful choices and thus ought to
either step back when an "informed" user invokes the tool or strictly
take the "least optimal, but correct" approach in strict mode.

Cheers
M
M
Maxime Devos wrote on 29 Jun 2022 12:18
754b54482ba7e443c587230b33ac56416f2b1eb9.camel@telenet.be
Liliana Marie Prikler schreef op wo 29-06-2022 om 12:15 [+0200]:
Toggle quote (8 lines)
> Here, it depends.  I think I'd write this as
>
>      (native-inputs 
>        (modify-inputs (package-native-inputs guile-3.0)
>          (prepend autoconf automake libtool
>                   flex gperf
>                   gnu-gettext texinfo)))

FWIW, I was thinking of

(native-inputs
(modify-inputs [...]
(prepend autoconf
automake
libtool
flex gperf
gnu-gettext
texinfo)))

, I haven't really thought about putting multiple inputs on a single
line myself.

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYrwm3xccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7kY8AP49dfhT3GaF6Uv4wpLEGxnN176/
gMDgCmUe4z1ECruagwEA8/qD8dO3djB5Mv9QRYK0LOXESpkHqgI2e0ADYEDCfQo=
=Mqbt
-----END PGP SIGNATURE-----


M
M
Maxime Devos wrote on 29 Jun 2022 12:19
473413ef0e01f1dcc7d62602a5b660230b216282.camel@telenet.be
Liliana Marie Prikler schreef op wo 29-06-2022 om 12:15 [+0200]:
Toggle quote (10 lines)
> > >                                     (delete-file
> > >                                      "test-
> > > suite/tests/version.test")
> > > #t))))))
> >
> > (Would be nice if "guix style" could be taught to remove those #t,
> > but
> > that seems more a feature limitation than a bug to me.)
> It can still do better by not contracting them imho.

TBC, do you mean doing #t -> #true, #f -> #false?

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYrwnHxccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7ht6AQD9TBZLgIz00TM9lBpxzw/Q5Q7o
Qj/27rcKfohwC2UMjQEA3HE8webHnUqijggjnp1mJv1wh1gvhhOk6Bqu0w1mIQw=
=U76J
-----END PGP SIGNATURE-----


L
L
Liliana Marie Prikler wrote on 29 Jun 2022 12:20
4de77339747298c76370c2a78ee7e0944a39078b.camel@ist.tugraz.at
Am Mittwoch, dem 29.06.2022 um 12:18 +0200 schrieb Maxime Devos:
Toggle quote (22 lines)
> Liliana Marie Prikler schreef op wo 29-06-2022 om 12:15 [+0200]:
> > Here, it depends.  I think I'd write this as
> >
> >      (native-inputs 
> >        (modify-inputs (package-native-inputs guile-3.0)
> >          (prepend autoconf automake libtool
> >                   flex gperf
> >                   gnu-gettext texinfo)))
>
> FWIW, I was thinking of
>
>    (native-inputs
>      (modify-inputs [...]
>        (prepend autoconf
>                 automake
>                 libtool
>                 flex gperf
>                 gnu-gettext
>                 texinfo)))
>
> , I haven't really thought about putting multiple inputs on a single
> line myself.
That ought to be the strict suggestion; the variant I posted above
should simply be seen as "acceptable" in lax mode for not breaking the
horizontal space limit.
L
L
Liliana Marie Prikler wrote on 29 Jun 2022 12:25
2a8343542ee2301f3effe1cdb04a121c180c119f.camel@ist.tugraz.at
Am Mittwoch, dem 29.06.2022 um 12:19 +0200 schrieb Maxime Devos:
Toggle quote (13 lines)
> Liliana Marie Prikler schreef op wo 29-06-2022 om 12:15 [+0200]:
> > > >                                     (delete-file
> > > >                                      "test-
> > > > suite/tests/version.test")
> > > > #t))))))
> > >
> > > (Would be nice if "guix style" could be taught to remove those
> > > #t,
> > > but
> > > that seems more a feature limitation than a bug to me.)
> > It can still do better by not contracting them imho.
>
> TBC, do you mean doing #t -> #true, #f -> #false?
I mean leaving them on an extra line. The current style tool mostly
errs when contracting multiple lines, which imho should not be its
task.

The problem that is solved here, is that people sometimes (particularly
in the uri field of the source) make these contractions for style
reasons. Guix style, having been taught that, tries to extrapolate
this to all fields.

Cheers
L
L
Ludovic Courtès wrote on 2 Jul 2022 12:11
Re: bug#56297: Guix style imperfections
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 56297@debbugs.gnu.org)
87sfnj259y.fsf@gnu.org
Hi,

Maxime Devos <maximedevos@telenet.be> skribis:

Toggle quote (3 lines)
> "guix style" occasionally makes some decision that seem a bit
> questionable to me.

Let’s keep in mind that some are bugs/limitations that can be fixed,
while others cannot really be addressed because our tastes vary
depending on context and the pretty printer could hardly be made smart
enough to distinguish all the subtleties.

Toggle quote (7 lines)
>> (define-public guile-next
>> (let ((version "3.0.7") (revision "0")
>> (commit "d70c1dbebf9ac0fd45af4578c23983ec4a7da535"))
>
> Conventionally 'revision' is put on another line -- for these kind of let bindings,
> (maybe all?), I would recommend to put all of them on separate lines.

This one is a bug IMO: ‘let’ bindings should be treated specially, and
currently they’re not.

Toggle quote (7 lines)
>> (substitute-keyword-arguments (package-arguments guile-3.0)
>> ((#:phases phases
>> '%standard-phases) `(modify-phases ,phases
>
> Put %standard-phases on the same line ad #:phases phases and `(modify-phases ,phases
> on a new lineg

OK.

Toggle quote (12 lines)
>> (add-before 'check 'skip-failing-tests
>> (lambda _
>> (substitute* "test-suite/standalone/test-out-of-memory"
>> (("!#") "!#
>>
>>(exit 77)
>>"))
>
> I'd prefer the original "!#\n\n(exit 77)\n" here, but I don't know if that's
> something 'Guix style' could feasibly do (there might be situations where a
> newline might be appropriate, how could "guix style" which is the case?).

Exactly: in synopses/descriptions, we do want to print newlines as-is
(see ‘tests/style.scm’).

Perhaps we could come up with heuristics that make different choices
depending on context, but that sounds tricky.

Toggle quote (6 lines)
>> (delete-file
>> "test-suite/tests/version.test") #t))))))
>
> (Would be nice if "guix style" could be taught to remove those #t, but that seems
> more a feature limitation than a bug to me.)

That could be the job of a different style rule (the ‘-S’ option).

Toggle quote (11 lines)
>> (native-inputs (modify-inputs (package-native-inputs guile-3.0)
>> (prepend autoconf
>> automake
>> libtool
>> flex
>> gnu-gettext
>> texinfo
>> gperf)))
>
> I'd consider it tidier to put (modify-inputs ...) on a new line

Dunno it’s a matter of taste. :-)

Ludo’.
L
L
Ludovic Courtès wrote on 4 Jul 2022 23:41
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 56297@debbugs.gnu.org)
87mtdo7dyj.fsf@gnu.org
Hi,

Ludovic Courtès <ludo@gnu.org> skribis:

Toggle quote (10 lines)
>>> (define-public guile-next
>>> (let ((version "3.0.7") (revision "0")
>>> (commit "d70c1dbebf9ac0fd45af4578c23983ec4a7da535"))
>>
>> Conventionally 'revision' is put on another line -- for these kind of let bindings,
>> (maybe all?), I would recommend to put all of them on separate lines.
>
> This one is a bug IMO: ‘let’ bindings should be treated specially, and
> currently they’re not.

Commit 8d9291bd2c36810be50ea340cefa481a42c60a2b fixes this, and…

Toggle quote (7 lines)
>>> (substitute-keyword-arguments (package-arguments guile-3.0)
>>> ((#:phases phases
>>> '%standard-phases) `(modify-phases ,phases
>>
>> Put %standard-phases on the same line ad #:phases phases and `(modify-phases ,phases
>> on a new lineg

… the second part of this one.

Ludo’.
M
M
Maxime Devos wrote on 19 Jul 2022 15:40
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 56297@debbugs.gnu.org)
a7faf1746e1139a13c77ce204e0d700cb41d639d.camel@telenet.be
Ludovic Courtès schreef op ma 04-07-2022 om 23:41 [+0200]:
Toggle quote (3 lines)
>
> Commit 8d9291bd2c36810be50ea340cefa481a42c60a2b fixes this [...]

Some issues remain, but nice!
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 56297@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 56297
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch