[PATCH 0/2] lint: Texinfo again.

[PATCH 0/2] lint: Texinfo again.

[Mathieu Lirzin]

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


As a follow up of

  <https://lists.gnu.org/archive/html/guix-devel/2015-09/msg00504.html>

I have tried to implement the requested changes.  There is a problem with
‘check-end-of-sentence-space’ because Texinfo conversion implicitly add
missing spaces so they are not detected by ‘guix lint’.  Maybe we should check
this on the raw description instead?


Mathieu Lirzin (2):
  lint: Check non-translated package descriptions.
  lint: Accept '`' character.

 guix/scripts/lint.scm | 19 +++++++++++--------
 guix/ui.scm           |  1 +
 2 files changed, 12 insertions(+), 8 deletions(-)

-- 
2.5.1

[PATCH 1/2] lint: Check non-translated package descriptions.

[Mathieu Lirzin]

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


* guix/ui.scm (texi->plain-text): Export.
* guix/scripts/lint.scm (check-description-style): Use it instead of
  'package-description-string'.
---
 guix/scripts/lint.scm | 17 ++++++++++-------
 guix/ui.scm           |  1 +
 2 files changed, 11 insertions(+), 7 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lint-Check-non-translated-package-descriptions.patch --]
[-- Type: text/x-patch; name="0001-lint-Check-non-translated-package-descriptions.patch", Size: 2046 bytes --]

diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index 8224f54..a52a9eb 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -143,12 +143,14 @@ monad."
                     (_ "description should not be empty")
                     'description)))
 
-  (define (check-texinfo-markup package)
-    "Check that PACKAGE description can be parsed as a Texinfo fragment."
+  (define (check-texinfo-markup description)
+    "Check that DESCRIPTION can be parsed as a Texinfo fragment.  If the
+markup is valid return a plain-text version of DESCRIPTION, otherwise #f."
     (catch 'parser-error
-      (lambda () (package-description-string package))
+      (lambda () (texi->plain-text description))
       (lambda (keys . args)
-        (emit-warning package (_ "Texinfo markup in description is invalid")))))
+        (emit-warning package (_ "Texinfo markup in description is invalid"))
+        #f)))
 
   (define (check-proper-start description)
     (unless (or (properly-starts-sentence? description)
@@ -179,9 +181,10 @@ by two spaces; possible infraction~p at ~{~a~^, ~}")
   (let ((description (package-description package)))
     (when (string? description)
       (check-not-empty description)
-      (check-texinfo-markup package)
-      (check-proper-start description)
-      (check-end-of-sentence-space description))))
+      (and=> (check-texinfo-markup description)
+             (lambda (texi)
+               (check-proper-start texi)
+               (check-end-of-sentence-space texi))))))
 
 (define (check-inputs-should-be-native package)
   ;; Emit a warning if some inputs of PACKAGE are likely to belong to its
diff --git a/guix/ui.scm b/guix/ui.scm
index 4a3630f..055f2c2 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -75,6 +75,7 @@
             switch-symlinks
             config-directory
             fill-paragraph
+            texi->plain-text
             package-description-string
             string->recutils
             package->recutils

[PATCH 2/2] lint: Accept '`' character.

[Mathieu Lirzin]

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


* guix/scripts/lint.scm (properly-starts-sentence?): Accept Texinfo
  highlighting commands as a sentence start.
---
 guix/scripts/lint.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-lint-Accept-character.patch --]
[-- Type: text/x-patch; name="0002-lint-Accept-character.patch", Size: 456 bytes --]

diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index a52a9eb..74989a6 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -129,7 +129,7 @@ monad."
   (exit 0))
 
 (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."

Re: [PATCH 1/2] lint: Check non-translated package descriptions.

[Ludovic Courtès]

Mathieu Lirzin <mthl@openmailbox.org> skribis:

> * guix/ui.scm (texi->plain-text): Export.
> * guix/scripts/lint.scm (check-description-style): Use it instead of
>   'package-description-string'.

[...]

> -      (check-texinfo-markup package)
> -      (check-proper-start description)
> -      (check-end-of-sentence-space description))))
> +      (and=> (check-texinfo-markup description)
> +             (lambda (texi)
> +               (check-proper-start texi)
> +               (check-end-of-sentence-space texi))))))

As you suggest, it’s best to keep ‘check-end-of-sentence-space’ before
Texinfo conversion if doing it after would be a no-op.

Could you do that and add a comment above?  OK to push with this change.

Thanks,
Ludo’.

Re: [PATCH 2/2] lint: Accept '`' character.

[Ludovic Courtès]

Mathieu Lirzin <mthl@openmailbox.org> skribis:

> * guix/scripts/lint.scm (properly-starts-sentence?): Accept Texinfo
>   highlighting commands as a sentence start.

OK, thanks!

Ludo’.

Re: [PATCH 1/2] lint: Check non-translated package descriptions.

[Mathieu Lirzin]

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

ludo@gnu.org (Ludovic Courtès) writes:

> Mathieu Lirzin <mthl@openmailbox.org> skribis:
>
>> * guix/ui.scm (texi->plain-text): Export.
>> * guix/scripts/lint.scm (check-description-style): Use it instead of
>>   'package-description-string'.
>
> [...]

After pushing it, I discovered one mistake here is the patch fixing
it...


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lint-Fix-missing-warning-symbol.patch --]
[-- Type: text/x-diff, Size: 1358 bytes --]

From 76f98cfc1567450913394ca871ebce40c8ed70e2 Mon Sep 17 00:00:00 2001
From: Mathieu Lirzin <mthl@openmailbox.org>
Date: Fri, 25 Sep 2015 00:37:36 +0200
Subject: [PATCH] lint: Fix missing warning symbol.

* guix/scripts/lint.scm (check-description-style): Set 'field' parameter
  when emitting a warning in 'check-texinfo-markup'.  This is a followup
  to commit 2748ee3.
---
 guix/scripts/lint.scm | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index b0427f7..0adb3bf 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -146,11 +146,11 @@ monad."
   (define (check-texinfo-markup description)
     "Check that DESCRIPTION can be parsed as a Texinfo fragment.  If the
 markup is valid return a plain-text version of DESCRIPTION, otherwise #f."
-    (catch 'parser-error
-      (lambda () (texi->plain-text description))
-      (lambda (keys . args)
-        (emit-warning package (_ "Texinfo markup in description is invalid"))
-        #f)))
+    (unless (false-if-exception (texi->plain-text description))
+      (emit-warning package
+                    (_ "Texinfo markup in description is invalid")
+                    'description)
+      #f))
 
   (define (check-proper-start description)
     (unless (or (properly-starts-sentence? description)
-- 
2.5.3


[-- Attachment #3: Type: text/plain, Size: 294 bytes --]


Usage of ‘false-if-exception’ made me realize that ‘emit-warning’ is not
nicely composable.  What about making it return ‘#f’ in order to compose
checks and warning together as boolean expressions?  Is that idiomatic?
Maybe you have a better suggestion?

--
Mathieu Lirzin

Re: [PATCH 1/2] lint: Check non-translated package descriptions.

[Ludovic Courtès]

Mathieu Lirzin <mthl@openmailbox.org> skribis:

> From 76f98cfc1567450913394ca871ebce40c8ed70e2 Mon Sep 17 00:00:00 2001
> From: Mathieu Lirzin <mthl@openmailbox.org>
> Date: Fri, 25 Sep 2015 00:37:36 +0200
> Subject: [PATCH] lint: Fix missing warning symbol.
>
> * guix/scripts/lint.scm (check-description-style): Set 'field' parameter
>   when emitting a warning in 'check-texinfo-markup'.  This is a followup
>   to commit 2748ee3.

This is only one part of what the patch does.

> diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
> index b0427f7..0adb3bf 100644
> --- a/guix/scripts/lint.scm
> +++ b/guix/scripts/lint.scm
> @@ -146,11 +146,11 @@ monad."
>    (define (check-texinfo-markup description)
>      "Check that DESCRIPTION can be parsed as a Texinfo fragment.  If the
>  markup is valid return a plain-text version of DESCRIPTION, otherwise #f."
> -    (catch 'parser-error
> -      (lambda () (texi->plain-text description))
> -      (lambda (keys . args)
> -        (emit-warning package (_ "Texinfo markup in description is invalid"))
> -        #f)))
> +    (unless (false-if-exception (texi->plain-text description))
> +      (emit-warning package
> +                    (_ "Texinfo markup in description is invalid")
> +                    'description)
> +      #f))

In general, it’s best to avoid ‘false-if-exception’ because it’s too
coarse-grain.  Here it’s probably OK though, because we want to catch
any error that may occur in during the conversion.  So this patch is OK
(with appropriate commit log.)

> Usage of ‘false-if-exception’ made me realize that ‘emit-warning’ is not
> nicely composable.  What about making it return ‘#f’ in order to compose
> checks and warning together as boolean expressions?  Is that idiomatic?
> Maybe you have a better suggestion?

Not sure I follow.  Those ‘check’ procedures are only called for effect,
not for value, right?

Thanks,
Ludo’.

Re: [PATCH 1/2] lint: Check non-translated package descriptions.

[Mathieu Lirzin]

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

ludo@gnu.org (Ludovic Courtès) writes:

> Mathieu Lirzin <mthl@openmailbox.org> skribis:
>
>> diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
>> index b0427f7..0adb3bf 100644
>> --- a/guix/scripts/lint.scm
>> +++ b/guix/scripts/lint.scm
>> @@ -146,11 +146,11 @@ monad."
>>    (define (check-texinfo-markup description)
>>      "Check that DESCRIPTION can be parsed as a Texinfo fragment.  If the
>>  markup is valid return a plain-text version of DESCRIPTION, otherwise #f."
>> -    (catch 'parser-error
>> -      (lambda () (texi->plain-text description))
>> -      (lambda (keys . args)
>> -        (emit-warning package (_ "Texinfo markup in description is invalid"))
>> -        #f)))
>> +    (unless (false-if-exception (texi->plain-text description))
>> +      (emit-warning package
>> +                    (_ "Texinfo markup in description is invalid")
>> +                    'description)
>> +      #f))
>
> In general, it’s best to avoid ‘false-if-exception’ because it’s too
> coarse-grain.  Here it’s probably OK though, because we want to catch
> any error that may occur in during the conversion.  So this patch is OK
> (with appropriate commit log.)

Ok, thanks for the explanation.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lint-Improve-check-texinfo-markup.patch --]
[-- Type: text/x-diff, Size: 1441 bytes --]

From 429ff285609120c4135eb64f1d6911924a24f5e6 Mon Sep 17 00:00:00 2001
From: Mathieu Lirzin <mthl@openmailbox.org>
Date: Fri, 25 Sep 2015 00:37:36 +0200
Subject: [PATCH] lint: Improve 'check-texinfo-markup'.

* guix/scripts/lint.scm (check-description-style): Set 'field' parameter
  when emitting a warning in 'check-texinfo-markup'.  Catch any error
  that may occur in during the 'texi->plain-text' conversion.  This is a
  followup to commit 2748ee3.
---
 guix/scripts/lint.scm | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index b0427f7..0adb3bf 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -146,11 +146,11 @@ monad."
   (define (check-texinfo-markup description)
     "Check that DESCRIPTION can be parsed as a Texinfo fragment.  If the
 markup is valid return a plain-text version of DESCRIPTION, otherwise #f."
-    (catch 'parser-error
-      (lambda () (texi->plain-text description))
-      (lambda (keys . args)
-        (emit-warning package (_ "Texinfo markup in description is invalid"))
-        #f)))
+    (unless (false-if-exception (texi->plain-text description))
+      (emit-warning package
+                    (_ "Texinfo markup in description is invalid")
+                    'description)
+      #f))
 
   (define (check-proper-start description)
     (unless (or (properly-starts-sentence? description)
-- 
2.5.3


[-- Attachment #3: Type: text/plain, Size: 1352 bytes --]


Are you OK with the new commit log?

>> Usage of ‘false-if-exception’ made me realize that ‘emit-warning’ is not
>> nicely composable.  What about making it return ‘#f’ in order to compose
>> checks and warning together as boolean expressions?  Is that idiomatic?
>> Maybe you have a better suggestion?
>
> Not sure I follow.  Those ‘check’ procedures are only called for effect,
> not for value, right?

My first idea was to have something similar to this kind of
composability...

--8<---------------cut here---------------start------------->8---
  (define (check-texinfo-markup description)
    (or (false-if-exception (texi->plain-text description))
        (emit-warning package
                      (_ "Texinfo markup in description is invalid")
                      'description)))

  (define (check-proper-start description)
    (or (properly-starts-sentence? description)
        (string-prefix-ci? (package-name package) description)
        (emit-warning package
                      (_ "description should start with an upper-case letter or digit")
                      'description)))
--8<---------------cut here---------------end--------------->8---

... but with better semantics.  IMO this would require more deep thought
so let's forget about this for now.  :)

--
Mathieu Lirzin

Re: [PATCH 1/2] lint: Check non-translated package descriptions.

[Ludovic Courtès]

Mathieu Lirzin <mthl@openmailbox.org> skribis:

> ludo@gnu.org (Ludovic Courtès) writes:
>
>> Mathieu Lirzin <mthl@openmailbox.org> skribis:
>>
>>> diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
>>> index b0427f7..0adb3bf 100644
>>> --- a/guix/scripts/lint.scm
>>> +++ b/guix/scripts/lint.scm
>>> @@ -146,11 +146,11 @@ monad."
>>>    (define (check-texinfo-markup description)
>>>      "Check that DESCRIPTION can be parsed as a Texinfo fragment.  If the
>>>  markup is valid return a plain-text version of DESCRIPTION, otherwise #f."
>>> -    (catch 'parser-error
>>> -      (lambda () (texi->plain-text description))
>>> -      (lambda (keys . args)
>>> -        (emit-warning package (_ "Texinfo markup in description is invalid"))
>>> -        #f)))
>>> +    (unless (false-if-exception (texi->plain-text description))
>>> +      (emit-warning package
>>> +                    (_ "Texinfo markup in description is invalid")
>>> +                    'description)
>>> +      #f))
>>
>> In general, it’s best to avoid ‘false-if-exception’ because it’s too
>> coarse-grain.  Here it’s probably OK though, because we want to catch
>> any error that may occur in during the conversion.  So this patch is OK
>> (with appropriate commit log.)
>
> Ok, thanks for the explanation.
>
>
> From 429ff285609120c4135eb64f1d6911924a24f5e6 Mon Sep 17 00:00:00 2001
> From: Mathieu Lirzin <mthl@openmailbox.org>
> Date: Fri, 25 Sep 2015 00:37:36 +0200
> Subject: [PATCH] lint: Improve 'check-texinfo-markup'.
>
> * guix/scripts/lint.scm (check-description-style): Set 'field' parameter
>   when emitting a warning in 'check-texinfo-markup'.  Catch any error
>   that may occur in during the 'texi->plain-text' conversion.  This is a
>   followup to commit 2748ee3.

OK!

Thanks,
Ludo’.

Re: [PATCH 1/2] lint: Check non-translated package descriptions.

[Mathieu Lirzin]

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

> Mathieu Lirzin <mthl@openmailbox.org> skribis:
>
>> From 429ff285609120c4135eb64f1d6911924a24f5e6 Mon Sep 17 00:00:00 2001
>> From: Mathieu Lirzin <mthl@openmailbox.org>
>> Date: Fri, 25 Sep 2015 00:37:36 +0200
>> Subject: [PATCH] lint: Improve 'check-texinfo-markup'.
>>
>> * guix/scripts/lint.scm (check-description-style): Set 'field' parameter
>>   when emitting a warning in 'check-texinfo-markup'.  Catch any error
>>   that may occur in during the 'texi->plain-text' conversion.  This is a
>>   followup to commit 2748ee3.

I should have tested my patch before pushing it :(

Sorry about that...


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lint-Fix-check-texinfo-markup.patch --]
[-- Type: text/x-diff, Size: 1425 bytes --]

From f491f62f16726d4b575db784781f50541e168b81 Mon Sep 17 00:00:00 2001
From: Mathieu Lirzin <mthl@openmailbox.org>
Date: Sat, 26 Sep 2015 18:47:37 +0200
Subject: [PATCH] lint: Fix 'check-texinfo-markup'.

Fixes a regression introduced in 5d8d8f3.

* guix/scripts/lint.scm (check-description-style): When no exception is
  thrown in 'check-texinfo-markup', return the rendered description.
---
 guix/scripts/lint.scm | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index 0adb3bf..3b4ff72 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -146,11 +146,13 @@ monad."
   (define (check-texinfo-markup description)
     "Check that DESCRIPTION can be parsed as a Texinfo fragment.  If the
 markup is valid return a plain-text version of DESCRIPTION, otherwise #f."
-    (unless (false-if-exception (texi->plain-text description))
-      (emit-warning package
-                    (_ "Texinfo markup in description is invalid")
-                    'description)
-      #f))
+    (catch #t
+      (lambda () (texi->plain-text description))
+      (lambda (keys . args)
+        (emit-warning package
+                      (_ "Texinfo markup in description is invalid")
+                      'description)
+        #f)))
 
   (define (check-proper-start description)
     (unless (or (properly-starts-sentence? description)
-- 
2.5.3


[-- Attachment #3: Type: text/plain, Size: 19 bytes --]


--
Mathieu Lirzin

Re: [PATCH 1/2] lint: Check non-translated package descriptions.

[Ludovic Courtès]

Mathieu Lirzin <mthl@openmailbox.org> skribis:

> From f491f62f16726d4b575db784781f50541e168b81 Mon Sep 17 00:00:00 2001
> From: Mathieu Lirzin <mthl@openmailbox.org>
> Date: Sat, 26 Sep 2015 18:47:37 +0200
> Subject: [PATCH] lint: Fix 'check-texinfo-markup'.
>
> Fixes a regression introduced in 5d8d8f3.
>
> * guix/scripts/lint.scm (check-description-style): When no exception is
>   thrown in 'check-texinfo-markup', return the rendered description.

OK!

Ludo'.