Re: guix lint false positives and RFC patch
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. OpenPGP_0x49E3EE22191725EE.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: guix lint false positives and RFC patch
On 2023-01-27, Simon Tournier wrote:
> On sam., 12 nov. 2022 at 17:54, Vagrant Cascadian wrote:
>> On 2022-11-05, Ludovic Courtès wrote:
>>> Vagrant Cascadian skribis:
From bfa13fdd3616839883e50efbbc05fb132610ce67 Mon Sep 17 00:00:00 2001
From: Vagrant Cascadian
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
signature.asc
Description: PGP signature
Re: guix lint false positives and RFC patch
Hi,
On sam., 12 nov. 2022 at 17:54, Vagrant Cascadian wrote:
> On 2022-11-05, Ludovic Courtès wrote:
>> Vagrant Cascadian skribis:
>>> From bfa13fdd3616839883e50efbbc05fb132610ce67 Mon Sep 17 00:00:00 2001
>>> From: Vagrant Cascadian
>>> 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
Re: guix lint false positives and RFC patch
Hi,
Vagrant Cascadian 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’.
Re: guix lint false positives and RFC patch
On 2022-11-05, Ludovic Courtès wrote:
> Vagrant Cascadian skribis:
>> From bfa13fdd3616839883e50efbbc05fb132610ce67 Mon Sep 17 00:00:00 2001
>> From: Vagrant Cascadian
>> 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
signature.asc
Description: PGP signature
Re: guix lint false positives and RFC patch
Hi,
Vagrant Cascadian skribis:
> From bfa13fdd3616839883e50efbbc05fb132610ce67 Mon Sep 17 00:00:00 2001
> From: Vagrant Cascadian
> 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’.
Re: guix lint false positives and RFC patch
On 2022-11-03, Vagrant Cascadian wrote:
> On 2022-11-03, Ludovic Courtès wrote:
>> Vagrant Cascadian 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
From bfa13fdd3616839883e50efbbc05fb132610ce67 Mon Sep 17 00:00:00 2001
From: Vagrant Cascadian
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
signature.asc
Description: PGP signature
Re: guix lint false positives and RFC patch
On 2022-11-03, Ludovic Courtès wrote:
> Vagrant Cascadian 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
From d4ddd4a90f18666352b07a4ebe0ed6b79c74f21e Mon Sep 17 00:00:00 2001
From: Vagrant Cascadian
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
signature.asc
Description: PGP signature
Re: guix lint false positives and RFC patch
Vagrant Cascadian 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’.
guix lint false positives and RFC patch
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
From 209c97b91d02831d78fc0b032f3b83fd5e0b9cb1 Mon Sep 17 00:00:00 2001
From: Vagrant Cascadian
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
signature.asc
Description: PGP signature
