[PATCH] lint: Warn about underscores in package names.

  • Done
  • quality assurance status badge
Details
4 participants
  • Julien Lepiller
  • Ludovic Courtès
  • Maxime Devos
  • Xinglu Chen
Owner
unassigned
Submitted by
Xinglu Chen
Severity
normal
X
X
Xinglu Chen wrote on 15 Apr 2021 18:00
(address . guix-patches@gnu.org)
dc3d1c690b40861eab4d623fb8bca88f4194e209.1618502119.git.public@yoctocell.xyz
As per section '16.4.2 Package Naming' in the manual, use hyphens
instead of underscores in package names.

* guix/lint.scm (check-name): Check whether the package name contains
underscores.
---
There was some discussion about this on guix-devel[1], but no consensus
was reached. Should we whitelist certain names like “86_64” or something?


guix/lint.scm | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)

Toggle diff (45 lines)
diff --git a/guix/lint.scm b/guix/lint.scm
index a7d6bbba4f..d5ad69475e 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -11,6 +11,7 @@
;;; Copyright © 2018, 2019 Arun Isaac <arunisaac@systemreboot.net>
;;; Copyright © 2020 Chris Marusich <cmmarusich@gmail.com>
;;; Copyright © 2020 Timothy Sample <samplet@ngyro.com>
+;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -173,14 +174,20 @@
(define (check-name package)
"Check whether PACKAGE's name matches our guidelines."
(let ((name (package-name package)))
- ;; Currently checks only whether the name is too short.
- (if (and (<= (string-length name) 1)
- (not (string=? name "r"))) ; common-sense exception
- (list
- (make-warning package
- (G_ "name should be longer than a single character")
- #:field 'name))
- '())))
+ (cond
+ ;; Currently checks only whether the name is too short.
+ ((and (<= (string-length name) 1)
+ (not (string=? name "r"))) ; common-sense exception
+ (list
+ (make-warning package
+ (G_ "name should be longer than a single character")
+ #:field 'name)))
+ ((string-match "_" name)
+ (list
+ (make-warning package
+ (G_ "name should use hyphens instead of underscores")
+ #:field 'name)))
+ (else '()))))
(define (properly-starts-sentence? s)
(string-match "^[(\"'`[:upper:][:digit:]]" s))

base-commit: a5bbd38fd131282e928144e869dcdf1e09259085
--
2.31.1
M
M
Maxime Devos wrote on 15 Apr 2021 18:19
e1a120c2fbd81850ec7a8281f6cc14cbdc0344fb.camel@telenet.be
On Thu, 2021-04-15 at 18:00 +0200, Xinglu Chen wrote:
Toggle quote (9 lines)
> As per section '16.4.2 Package Naming' in the manual, use hyphens
> instead of underscores in package names.
>
> * guix/lint.scm (check-name): Check whether the package name contains
> underscores.
> ---
> There was some discussion about this on guix-devel[1], but no consensus
> was reached. Should we whitelist certain names like “86_64” or something?

I dunno. Perhaps for now, we can accept that the 'check-name' linter is
sometimes overly strict, and punt the exceptions for later?

Toggle quote (44 lines)
>
> guix/lint.scm | 23 +++++++++++++++--------
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/guix/lint.scm b/guix/lint.scm
> index a7d6bbba4f..d5ad69475e 100644
> --- a/guix/lint.scm
> +++ b/guix/lint.scm
> @@ -11,6 +11,7 @@
> ;;; Copyright © 2018, 2019 Arun Isaac <arunisaac@systemreboot.net>
> ;;; Copyright © 2020 Chris Marusich <cmmarusich@gmail.com>
> ;;; Copyright © 2020 Timothy Sample <samplet@ngyro.com>
> +;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz>
> ;;;
> ;;; This file is part of GNU Guix.
> ;;;
> @@ -173,14 +174,20 @@
> (define (check-name package)
> "Check whether PACKAGE's name matches our guidelines."
> (let ((name (package-name package)))
> - ;; Currently checks only whether the name is too short.
> - (if (and (<= (string-length name) 1)
> - (not (string=? name "r"))) ; common-sense exception
> - (list
> - (make-warning package
> - (G_ "name should be longer than a single character")
> - #:field 'name))
> - '())))
> + (cond
> + ;; Currently checks only whether the name is too short.
> + ((and (<= (string-length name) 1)
> + (not (string=? name "r"))) ; common-sense exception
> + (list
> + (make-warning package
> + (G_ "name should be longer than a single character")
> + #:field 'name)))
> + ((string-match "_" name)
> + (list
> + (make-warning package
> + (G_ "name should use hyphens instead of underscores")
> + #:field 'name)))
> + (else '()))))

I didn't test this, but that seems about right.
Ideally, you should add a corresponding test in tests/lint.scm
(IIRC, maybe the linter tests are elsewhere).

However, Guix is currently in the "string freeze", so this cannot yet be merged,
IIUC.

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

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYHhnqRccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7qHoAQCJkjMTs4202E/7IFSOPxia4GVu
4OHuE4d2j4Vici9O0QEAp56CBgX7ASGNK2RUFme7pCmtXujKI6TxpHcpx/804QE=
=pJJG
-----END PGP SIGNATURE-----


X
X
Xinglu Chen wrote on 15 Apr 2021 20:15
8735vrh0t6.fsf@yoctocell.xyz
On Thu, Apr 15 2021, Maxime Devos wrote:

Toggle quote (7 lines)
>> There was some discussion about this on guix-devel[1], but no
>> consensus was reached. Should we whitelist certain names like
>> “86_64” or something?
>
> I dunno. Perhaps for now, we can accept that the 'check-name' linter
> is sometimes overly strict, and punt the exceptions for later?

Ok, that sounds like a good idea.

Toggle quote (4 lines)
> I didn't test this, but that seems about right. Ideally, you should
> add a corresponding test in tests/lint.scm (IIRC, maybe the linter
> tests are elsewhere).

Will do.

Toggle quote (3 lines)
> However, Guix is currently in the "string freeze", so this cannot yet
> be merged, IIUC.

I thought the “string freeze” was for the manual, or did I miss
something?

Thanks for the review!
X
X
Xinglu Chen wrote on 15 Apr 2021 20:31
[PATCH v2] lint: Warn about underscores in package names.
(address . guix-patches@gnu.org)(name . Maxime Devos)(address . maximedevos@telenet.be)
94d0b8cb2322ed634442c0ebb79589e5dc05e526.1618511382.git.public@yoctocell.xyz
As per section '16.4.2 Package Naming' in the manual, use hyphens
instead of underscores in package names.

* guix/lint.scm (check-name): Check whether the package name contains
underscores.
* tests/lint.scm ("name: use underscore in package name"): New test.
---
Changes since v1:
- Add test for package name with underscore.

guix/lint.scm | 24 ++++++++++++++++--------
tests/lint.scm | 7 +++++++
2 files changed, 23 insertions(+), 8 deletions(-)

Toggle diff (78 lines)
diff --git a/guix/lint.scm b/guix/lint.scm
index a7d6bbba4f..38699e2927 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -11,6 +11,7 @@
;;; Copyright © 2018, 2019 Arun Isaac <arunisaac@systemreboot.net>
;;; Copyright © 2020 Chris Marusich <cmmarusich@gmail.com>
;;; Copyright © 2020 Timothy Sample <samplet@ngyro.com>
+;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -81,6 +82,7 @@
check-synopsis-style
check-derivation
check-home-page
+ check-name
check-source
check-source-file-name
check-source-unstable-tarball
@@ -173,14 +175,20 @@
(define (check-name package)
"Check whether PACKAGE's name matches our guidelines."
(let ((name (package-name package)))
- ;; Currently checks only whether the name is too short.
- (if (and (<= (string-length name) 1)
- (not (string=? name "r"))) ; common-sense exception
- (list
- (make-warning package
- (G_ "name should be longer than a single character")
- #:field 'name))
- '())))
+ (cond
+ ;; Currently checks only whether the name is too short.
+ ((and (<= (string-length name) 1)
+ (not (string=? name "r"))) ; common-sense exception
+ (list
+ (make-warning package
+ (G_ "name should be longer than a single character")
+ #:field 'name)))
+ ((string-match "_" name)
+ (list
+ (make-warning package
+ (G_ "name should use hyphens instead of underscores")
+ #:field 'name)))
+ (else '()))))
(define (properly-starts-sentence? s)
(string-match "^[(\"'`[:upper:][:digit:]]" s))
diff --git a/tests/lint.scm b/tests/lint.scm
index bd8604f589..a2c8665142 100644
--- a/tests/lint.scm
+++ b/tests/lint.scm
@@ -8,6 +8,7 @@
;;; Copyright © 2017 Efraim Flashner <efraim@flashner.co.il>
;;; Copyright © 2018, 2019 Arun Isaac <arunisaac@systemreboot.net>
;;; Copyright © 2020 Timothy Sample <samplet@ngyro.com>
+;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -270,6 +271,12 @@
(description "Imagine this is Taylor UUCP."))))
(check-synopsis-style pkg)))
+(test-equal "name: use underscore in package name"
+ "name should use hyphens instead of underscores"
+ (single-lint-warning-message
+ (let ((pkg (dummy-package "under_score")))
+ (check-name pkg))))
+
(test-equal "inputs: pkg-config is probably a native input"
"'pkg-config' should probably be a native input"
(single-lint-warning-message

base-commit: a5bbd38fd131282e928144e869dcdf1e09259085
--
2.31.1
M
M
Maxime Devos wrote on 16 Apr 2021 12:19
Re: [bug#47804] [PATCH] lint: Warn about underscores in package names.
762053ed2175f1ded96da8d85855843d73936364.camel@telenet.be
On Thu, 2021-04-15 at 20:15 +0200, Xinglu Chen wrote:
Toggle quote (9 lines)
> On Thu, Apr 15 2021, Maxime Devos wrote:
> [...]
>
> > However, Guix is currently in the "string freeze", so this cannot yet
> > be merged, IIUC.
>
> I thought the “string freeze” was for the manual, or did I miss
> something?

The freeze if for both the manual and guix itself (but not the packages).

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

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYHlkuxccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7lSBAQCIDxvJ6k1JE0Eqj3baGyyz6uB5
PuXkpzcu20JXXLv7MQD/bsGmJZOwzsa2bGecPOdtIwbN18VF8ZEP140SvpbRUQA=
=btmY
-----END PGP SIGNATURE-----


L
L
Ludovic Courtès wrote on 16 Apr 2021 22:54
Re: bug#47804: [PATCH] lint: Warn about underscores in package names.
(name . Xinglu Chen)(address . public@yoctocell.xyz)
87eefa2bmw.fsf_-_@gnu.org
Hi,

Xinglu Chen <public@yoctocell.xyz> skribis:

Toggle quote (7 lines)
> As per section '16.4.2 Package Naming' in the manual, use hyphens
> instead of underscores in package names.
>
> * guix/lint.scm (check-name): Check whether the package name contains
> underscores.
> * tests/lint.scm ("name: use underscore in package name"): New test.

Applied with the minor change below, which avoids regexps
(‘string-match’ performs regexp matches, which is overkill here).

Thank you and thanks Maxime for the review!

Ludo’.
Toggle diff (13 lines)
diff --git a/guix/lint.scm b/guix/lint.scm
index 38699e2927..1bebfe03d3 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -183,7 +183,7 @@
(make-warning package
(G_ "name should be longer than a single character")
#:field 'name)))
- ((string-match "_" name)
+ ((string-index name #\_)
(list
(make-warning package
(G_ "name should use hyphens instead of underscores")
Closed
J
J
Julien Lepiller wrote on 16 Apr 2021 23:42
E609DF3C-68B3-4A32-B103-5C38F64834E0@lepiller.eu
Le 16 avril 2021 16:54:47 GMT-04:00, "Ludovic Courtès" <ludo@gnu.org> a écrit :
Toggle quote (18 lines)
>Hi,
>
>Xinglu Chen <public@yoctocell.xyz> skribis:
>
>> As per section '16.4.2 Package Naming' in the manual, use hyphens
>> instead of underscores in package names.
>>
>> * guix/lint.scm (check-name): Check whether the package name contains
>> underscores.
>> * tests/lint.scm ("name: use underscore in package name"): New test.
>
>Applied with the minor change below, which avoids regexps
>(‘string-match’ performs regexp matches, which is overkill here).
>
>Thank you and thanks Maxime for the review!
>
>Ludo’.

Hm… unless I'm mistaken, this patch adds a string to the guix domain, which is part of the string freeze.

The guix-packages is not part of the string freeze, that is to say synopsis and description of packages, only.
?