[-- 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 --]
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’.
[-- 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 --]
[-- 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 --]
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’.
[-- 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 --]
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’.
Hi, On sam., 12 nov. 2022 at 17:54, Vagrant Cascadian <vagrant@debian.org> wrote: > 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... [...] > What is failing to match what here? Well, almost done but not merged, right? Still an issue this ’match’? Cheers, simon
[-- Attachment #1: Type: text/plain, Size: 2333 bytes --] On 2023-01-27, Simon Tournier wrote: > On sam., 12 nov. 2022 at 17:54, Vagrant Cascadian <vagrant@debian.org> wrote: >> 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... > > [...] > >> What is failing to match what here? > > Well, almost done but not merged, right? > > Still an issue this ’match’? Thanks for bringing this back up to the surface! I struggled with it a bit and honestly do not remember where I last left it... I think I made some further progress... and then hit a new blocker and the sun went down and slept on it ... and never got back to it. So I was definitely stuck writing a test. From vague memory, I think once I figured out how to have the test not fail wit guile complaining inscrutibly... it did not effectively test the thing it was supposed to. The other thing I remember being caught up on, which was not a deal-breaker, per se, was hoping for a way to loop through a bunch of @SOMETHING things ... I was not happy with: + (if (>= (string-length (string-replace-substring + (string-replace-substring synopsis "@acronym" "") + "@code" "")) + 80) And then adding @command, @file, @acronym, etc. ... using increasingly nested levels string-replace-substring would eventually become difficult to read and surely there is a better way! I might be able to take another look at this in february, but I would welcome help wrapping this up regardless! live well, vagrant [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 227 bytes --]
[-- Attachment #1.1.1: Type: text/plain, Size: 917 bytes --] On 28-01-2023 22:07, Vagrant Cascadian wrote: > The other thing I remember being caught up on, which was not a > deal-breaker, per se, was hoping for a way to loop through a bunch of > @SOMETHING things ... I was not happy with: > > + (if (>= (string-length (string-replace-substring > + (string-replace-substring synopsis "@acronym" "") > + "@code" "")) > + 80) > > And then adding @command, @file, @acronym, etc. ... using increasingly > nested levels string-replace-substring would eventually become difficult > to read and surely there is a better way! How about some regex: (define regexp (delay (make-regexp "@(code|acronym|file)\\b"))) (define example "stuff @acronym @code @file") (regexp-substitute/global #f (force regexp) example 'pre "" 'post) $4 = "stuff ". Greetings, Maxime. [-- Attachment #1.1.2: OpenPGP public key --] [-- Type: application/pgp-keys, Size: 929 bytes --] [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 236 bytes --]