unofficial mirror of guix-devel@gnu.org 
 help / color / mirror / code / Atom feed
* guix lint false positives and RFC patch
@ 2022-11-03  3:29 Vagrant Cascadian
  2022-11-03 15:20 ` Ludovic Courtès
  0 siblings, 1 reply; 7+ messages in thread
From: Vagrant Cascadian @ 2022-11-03  3:29 UTC (permalink / raw)
  To: guix-devel


[-- Attachment #1.1: Type: text/plain, Size: 627 bytes --]

I've noticed a handful of false positives in guix lint checking
descriptions and synopsis, and tracked several down to the use of
@code{} and similar.

The attached patch partly addresses this, though could definitely be
written better (e.g. handling more cases, also stripping out the
relevent "}", etc.)

This fixes about 11 out of 544 overall guix lint issues with
descriptions and synopsis. Had expected it to fix more issues, but I
think stripping the "@code{" reveals issues in other checks that were
previously hidden.... maybe.

I will reiterate that this leaves a lot of room for improvement. :)

live well,
  vagrant

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-guix-lint-Exclude-code-from-various-checks.patch --]
[-- Type: text/x-diff, Size: 1530 bytes --]

From 209c97b91d02831d78fc0b032f3b83fd5e0b9cb1 Mon Sep 17 00:00:00 2001
From: Vagrant Cascadian <vagrant@debian.org>
Date: Wed, 2 Nov 2022 19:56:12 -0700
Subject: [PATCH] guix: lint: Exclude "@code{" from various checks.

The visual representation of the relevent description and synopsis do not
include the string, so exclude it from checks to avoid false positives.

FIXME handle @command, @file, @acronym, etc.

* guix/linx.scm (properly-starts-sentence): Exclude leading "@code{".
  (check-synopsis-length): Exclude "@code{".
---
 guix/lint.scm | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/guix/lint.scm b/guix/lint.scm
index 8e3976171f..4dc35218db 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -313,7 +313,8 @@ (define (tests-explicitly-enabled?)
       '()))
 
 (define (properly-starts-sentence? s)
-  (string-match "^[(\"'`[:upper:][:digit:]]" s))
+  (string-match "^[(\"'`[:upper:][:digit:]]"
+                (string-replace-substring s "@code{" "")))
 
 (define (starts-with-abbreviation? s)
   "Return #t if S starts with what looks like an abbreviation or acronym."
@@ -650,7 +651,7 @@ (define check-start-article
               '()))))
 
   (define (check-synopsis-length synopsis)
-    (if (>= (string-length synopsis) 80)
+    (if (>= (string-length (string-replace-substring synopsis "@code{" "")) 80)
         (list
          (make-warning package
                        (G_ "synopsis should be less than 80 characters long")
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: guix lint false positives and RFC patch
  2022-11-03  3:29 guix lint false positives and RFC patch Vagrant Cascadian
@ 2022-11-03 15:20 ` Ludovic Courtès
  2022-11-03 23:46   ` Vagrant Cascadian
  0 siblings, 1 reply; 7+ messages in thread
From: Ludovic Courtès @ 2022-11-03 15:20 UTC (permalink / raw)
  To: Vagrant Cascadian; +Cc: guix-devel

Vagrant Cascadian <vagrant@debian.org> skribis:

> --- a/guix/lint.scm
> +++ b/guix/lint.scm
> @@ -313,7 +313,8 @@ (define (tests-explicitly-enabled?)
>        '()))
>  
>  (define (properly-starts-sentence? s)
> -  (string-match "^[(\"'`[:upper:][:digit:]]" s))
> +  (string-match "^[(\"'`[:upper:][:digit:]]"
> +                (string-replace-substring s "@code{" "")))

An identifier in @code or file name in @file may legitimately start with
a lower-case letter so I don’t think we should try to prevent that.

However, maybe we could change the regexp above to something that
accepts @ or some other non-letter character at the start?

Ludo’.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: guix lint false positives and RFC patch
  2022-11-03 15:20 ` Ludovic Courtès
@ 2022-11-03 23:46   ` Vagrant Cascadian
  2022-11-04 22:24     ` Vagrant Cascadian
  0 siblings, 1 reply; 7+ messages in thread
From: Vagrant Cascadian @ 2022-11-03 23:46 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: guix-devel


[-- Attachment #1.1: Type: text/plain, Size: 1343 bytes --]

On 2022-11-03, Ludovic Courtès wrote:
> Vagrant Cascadian <vagrant@debian.org> skribis:
>
>> --- a/guix/lint.scm
>> +++ b/guix/lint.scm
>> @@ -313,7 +313,8 @@ (define (tests-explicitly-enabled?)
>>        '()))
>>  
>>  (define (properly-starts-sentence? s)
>> -  (string-match "^[(\"'`[:upper:][:digit:]]" s))
>> +  (string-match "^[(\"'`[:upper:][:digit:]]"
>> +                (string-replace-substring s "@code{" "")))
>
> An identifier in @code or file name in @file may legitimately start with
> a lower-case letter so I don’t think we should try to prevent that.
>
> However, maybe we could change the regexp above to something that
> accepts @ or some other non-letter character at the start?

Great suggestion, as it is simpler, easier to read, and actually covers
more false positives! Updated patch attached!

I think there was only one case fixed by the @code{} check for the
synopsis length check, and I don't see any obvious other @*{} checks
that would currently improve anything, though it would be ideal to make
it more future-proof just in case... though I think my attempt at that
would be awfully ugly, help from others would be welcome!

That said, I think this resolves 52 false positives with guix lint (~10%
of the 536 synopsis/description issues currently).


live well,
  vagrant

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-guix-lint-Exclude-some-symbols-from-various-checks.patch --]
[-- Type: text/x-diff, Size: 1492 bytes --]

From d4ddd4a90f18666352b07a4ebe0ed6b79c74f21e Mon Sep 17 00:00:00 2001
From: Vagrant Cascadian <vagrant@debian.org>
Date: Wed, 2 Nov 2022 19:56:12 -0700
Subject: [PATCH] guix: lint: Exclude some "@" symbols from various checks.

The visual representation of "@code{}" or similar in the description and
synopsis do not include the string, so exclude it from checks to avoid false
positives.

FIXME handle @command, @file, @acronym, etc.

* guix/linx.scm (properly-starts-sentence): Exclude leading "@".
  (check-synopsis-length): Exclude "@code{".
---
 guix/lint.scm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/guix/lint.scm b/guix/lint.scm
index 8e3976171f..26d8f59a2c 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -313,7 +313,7 @@ (define (tests-explicitly-enabled?)
       '()))
 
 (define (properly-starts-sentence? s)
-  (string-match "^[(\"'`[:upper:][:digit:]]" s))
+  (string-match "^[@(\"'`[:upper:][:digit:]]" s))
 
 (define (starts-with-abbreviation? s)
   "Return #t if S starts with what looks like an abbreviation or acronym."
@@ -650,7 +650,7 @@ (define check-start-article
               '()))))
 
   (define (check-synopsis-length synopsis)
-    (if (>= (string-length synopsis) 80)
+    (if (>= (string-length (string-replace-substring synopsis "@code{" "")) 80)
         (list
          (make-warning package
                        (G_ "synopsis should be less than 80 characters long")
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: guix lint false positives and RFC patch
  2022-11-03 23:46   ` Vagrant Cascadian
@ 2022-11-04 22:24     ` Vagrant Cascadian
  2022-11-05 17:52       ` Ludovic Courtès
  0 siblings, 1 reply; 7+ messages in thread
From: Vagrant Cascadian @ 2022-11-04 22:24 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: guix-devel


[-- Attachment #1.1: Type: text/plain, Size: 1695 bytes --]

On 2022-11-03, Vagrant Cascadian wrote:
> On 2022-11-03, Ludovic Courtès wrote:
>> Vagrant Cascadian <vagrant@debian.org> skribis:
>>
>>> --- a/guix/lint.scm
>>> +++ b/guix/lint.scm
>>> @@ -313,7 +313,8 @@ (define (tests-explicitly-enabled?)
>>>        '()))
>>>  
>>>  (define (properly-starts-sentence? s)
>>> -  (string-match "^[(\"'`[:upper:][:digit:]]" s))
>>> +  (string-match "^[(\"'`[:upper:][:digit:]]"
>>> +                (string-replace-substring s "@code{" "")))
>>
>> An identifier in @code or file name in @file may legitimately start with
>> a lower-case letter so I don’t think we should try to prevent that.
>>
>> However, maybe we could change the regexp above to something that
>> accepts @ or some other non-letter character at the start?
>
> Great suggestion, as it is simpler, easier to read, and actually covers
> more false positives! Updated patch attached!
>
> I think there was only one case fixed by the @code{} check for the
> synopsis length check, and I don't see any obvious other @*{} checks
> that would currently improve anything, though it would be ideal to make
> it more future-proof just in case... though I think my attempt at that
> would be awfully ugly, help from others would be welcome!

Well, ugly is what I have... Found two packages affected by @acronym.

Also realized that it should leave the {} in the length-matching, as
they are typically replaced by other characters when rendered.


> That said, I think this resolves 52 false positives with guix lint (~10%
> of the 536 synopsis/description issues currently).

Think with this applied there are 54 false positives fixed.

live well,
  vagrant

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-guix-lint-Exclude-some-symbols-from-various-checks.patch --]
[-- Type: text/x-diff, Size: 1647 bytes --]

From bfa13fdd3616839883e50efbbc05fb132610ce67 Mon Sep 17 00:00:00 2001
From: Vagrant Cascadian <vagrant@debian.org>
Date: Wed, 2 Nov 2022 19:56:12 -0700
Subject: [PATCH 01/12] guix: lint: Exclude some "@" symbols from various
 checks.

The visual representation of "@code{}" or similar in the description and
synopsis do not include the string, so exclude it from checks to avoid false
positives.

FIXME handle @command, @file, @acronym, etc.

* guix/linx.scm (properly-starts-sentence): Exclude leading "@".
  (check-synopsis-length): Exclude "@code" and "@acronym".
---
 guix/lint.scm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/guix/lint.scm b/guix/lint.scm
index 8e3976171f..76b17b297d 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -313,7 +313,7 @@ (define (tests-explicitly-enabled?)
       '()))
 
 (define (properly-starts-sentence? s)
-  (string-match "^[(\"'`[:upper:][:digit:]]" s))
+  (string-match "^[@(\"'`[:upper:][:digit:]]" s))
 
 (define (starts-with-abbreviation? s)
   "Return #t if S starts with what looks like an abbreviation or acronym."
@@ -650,7 +650,10 @@ (define check-start-article
               '()))))
 
   (define (check-synopsis-length synopsis)
-    (if (>= (string-length synopsis) 80)
+    (if (>= (string-length (string-replace-substring
+                            (string-replace-substring synopsis "@acronym" "")
+                            "@code" ""))
+                           80)
         (list
          (make-warning package
                        (G_ "synopsis should be less than 80 characters long")
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: guix lint false positives and RFC patch
  2022-11-04 22:24     ` Vagrant Cascadian
@ 2022-11-05 17:52       ` Ludovic Courtès
  2022-11-13  1:54         ` Vagrant Cascadian
  0 siblings, 1 reply; 7+ messages in thread
From: Ludovic Courtès @ 2022-11-05 17:52 UTC (permalink / raw)
  To: Vagrant Cascadian; +Cc: guix-devel

Hi,

Vagrant Cascadian <vagrant@debian.org> skribis:

> From bfa13fdd3616839883e50efbbc05fb132610ce67 Mon Sep 17 00:00:00 2001
> From: Vagrant Cascadian <vagrant@debian.org>
> Date: Wed, 2 Nov 2022 19:56:12 -0700
> Subject: [PATCH 01/12] guix: lint: Exclude some "@" symbols from various
>  checks.
>
> The visual representation of "@code{}" or similar in the description and
> synopsis do not include the string, so exclude it from checks to avoid false
> positives.
>
> FIXME handle @command, @file, @acronym, etc.
>
> * guix/linx.scm (properly-starts-sentence): Exclude leading "@".
>   (check-synopsis-length): Exclude "@code" and "@acronym".

LGTM!  Bonus points for a test in ‘tests/lint.scm’.  :-)

Ludo’.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: guix lint false positives and RFC patch
  2022-11-05 17:52       ` Ludovic Courtès
@ 2022-11-13  1:54         ` Vagrant Cascadian
  2022-11-17 15:03           ` Ludovic Courtès
  0 siblings, 1 reply; 7+ messages in thread
From: Vagrant Cascadian @ 2022-11-13  1:54 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: guix-devel

[-- Attachment #1: Type: text/plain, Size: 3114 bytes --]

On 2022-11-05, Ludovic Courtès wrote:
> Vagrant Cascadian <vagrant@debian.org> skribis:
>> From bfa13fdd3616839883e50efbbc05fb132610ce67 Mon Sep 17 00:00:00 2001
>> From: Vagrant Cascadian <vagrant@debian.org>
>> Date: Wed, 2 Nov 2022 19:56:12 -0700
>> Subject: [PATCH 01/12] guix: lint: Exclude some "@" symbols from various
>>  checks.
>>
>> The visual representation of "@code{}" or similar in the description and
>> synopsis do not include the string, so exclude it from checks to avoid false
>> positives.
>>
>> FIXME handle @command, @file, @acronym, etc.
>>
>> * guix/linx.scm (properly-starts-sentence): Exclude leading "@".
>>   (check-synopsis-length): Exclude "@code" and "@acronym".
>
> LGTM!  Bonus points for a test in ‘tests/lint.scm’.  :-)

No bonus points for me just yet...

diff --git a/tests/lint.scm b/tests/lint.scm
index ce22e2355a..26e93ca37b 100644
--- a/tests/lint.scm
+++ b/tests/lint.scm
@@ -283,6 +283,16 @@ (define (warning-contains? str warnings)
                              (synopsis (make-string 80 #\X)))))
      (check-synopsis-style pkg))))

+(test-equal "synopsis: exclude @code from long synopsis"
+  '()
+  (single-lint-warning-message
+   (let ((pkg (dummy-package "x"
+                             (synopsis
+                              (string-append
+                                        "@code{X}"
+                                        (make-string 72 #\X))))))
+     (check-synopsis-style pkg))))
+
 (test-equal "synopsis: start with package name"
   "synopsis should not start with the package name"
   (single-lint-warning-message

The above test doesn't catch this issue, even though the code works on
real packages... I am a bit stumped as to why. I guess '() (or "" which
I also tried) is not a valid way to try to express "this test expects no
warning/error/message/etc."?

Here is a log from the test I cargo-culted:

test-name: synopsis: too long
location: /home/vagrant/src/guix/tests/lint.scm:279
source:
+ (test-equal
+   "synopsis: too long"
+   "synopsis should be less than 80 characters long"
+   (single-lint-warning-message
+     (let ((pkg (dummy-package
+                  "x"
+                  (synopsis (make-string 80 #\X)))))
+       (check-synopsis-style pkg))))
expected-value: "synopsis should be less than 80 characters long"
actual-value: "synopsis should be less than 80 characters long"
result: PASS

And from my test in the patch listed above:

test-name: synopsis: exclude @code from long synopsis
location: /home/vagrant/src/guix/tests/lint.scm:286
source:
+ (test-equal
+   "synopsis: exclude @code from long synopsis"
+   '()
+   (single-lint-warning-message
+     (let ((pkg (dummy-package
+                  "x"
+                  (synopsis
+                    (string-append "@code{X}" (make-string 72 #\X))))))
+       (check-synopsis-style pkg))))
expected-value: ()
actual-value: #f
actual-error:
+ (match-error "match" "no matching pattern" ())
result: FAIL


What is failing to match what here?


live well,
  vagrant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: guix lint false positives and RFC patch
  2022-11-13  1:54         ` Vagrant Cascadian
@ 2022-11-17 15:03           ` Ludovic Courtès
  0 siblings, 0 replies; 7+ messages in thread
From: Ludovic Courtès @ 2022-11-17 15:03 UTC (permalink / raw)
  To: Vagrant Cascadian; +Cc: guix-devel

Hi,

Vagrant Cascadian <vagrant@debian.org> skribis:

> +(test-equal "synopsis: exclude @code from long synopsis"
> +  '()
> +  (single-lint-warning-message
> +   (let ((pkg (dummy-package "x"
> +                             (synopsis
> +                              (string-append
> +                                        "@code{X}"
> +                                        (make-string 72 #\X))))))
> +     (check-synopsis-style pkg))))

If you’re expecting an empty list (no warnings), remove the call to
‘single-lint-warning-message’: that one assumes you’re expecting exactly
one lint warning.

HTH!

Ludo’.


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-11-17 15:03 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-03  3:29 guix lint false positives and RFC patch Vagrant Cascadian
2022-11-03 15:20 ` Ludovic Courtès
2022-11-03 23:46   ` Vagrant Cascadian
2022-11-04 22:24     ` Vagrant Cascadian
2022-11-05 17:52       ` Ludovic Courtès
2022-11-13  1:54         ` Vagrant Cascadian
2022-11-17 15:03           ` Ludovic Courtès

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).