diff mbox series

[bug#35790,2/2] lint: Separate checkers by dependence on the internet.

Message ID 20190702192542.16179-2-mail@cbaines.net
State Accepted
Headers show
Series [bug#35790,1/2] lint: Move the linting code to a different module. | expand

Commit Message

Christopher Baines July 2, 2019, 7:25 p.m. UTC 
I think there are a couple of potential uses for this. It's somewhat a
separation in to what checkers are just checking the contents of the
repository (line length for example), and other checkers which are bringing in
external information which could change.

I'm thinking particularly, about treating network dependant checkers
differently when automatically running them, but this commit also adds a
--no-network flag to guix lint, which selects the checkers that don't access
the network, which could be useful if no network access is available.

* guix/lint.scm (%checkers): Rename to %all-checkers.
(%local-checkers, %network-dependant-checkers): New variables.
* guix/scripts/lint.scm (run-checkers): Make the checkers argument mandatory.
(list-checkers-and-exit): Handle the checkers as an argument.
(%options): Adjust for changes to %checkers, add a --no-network option, and
change how the --list-checkers option is handled.
(guix-lint): Adjust indentation, and update how the checkers are handled.
---
 guix/lint.scm         | 64 +++++++++++++++++++++++++------------------
 guix/scripts/lint.scm | 49 ++++++++++++++++++++-------------
 2 files changed, 67 insertions(+), 46 deletions(-)

Comments

Ludovic Courtès July 12, 2019, 2:38 p.m. UTC | #1 
Hi,

Christopher Baines <mail@cbaines.net> skribis:

> I think there are a couple of potential uses for this. It's somewhat a
> separation in to what checkers are just checking the contents of the
> repository (line length for example), and other checkers which are bringing in
> external information which could change.
>
> I'm thinking particularly, about treating network dependant checkers
> differently when automatically running them, but this commit also adds a
> --no-network flag to guix lint, which selects the checkers that don't access
> the network, which could be useful if no network access is available.
>
> * guix/lint.scm (%checkers): Rename to %all-checkers.
> (%local-checkers, %network-dependant-checkers): New variables.
> * guix/scripts/lint.scm (run-checkers): Make the checkers argument mandatory.
> (list-checkers-and-exit): Handle the checkers as an argument.
> (%options): Adjust for changes to %checkers, add a --no-network option, and
> change how the --list-checkers option is handled.
> (guix-lint): Adjust indentation, and update how the checkers are handled.

Nice.

> +(define %network-dependant-checkers
                          ^
Shouldn’t it be “dependent” with an ‘e’?

Otherwise LGTM, thanks!

Ludo’.
Christopher Baines July 14, 2019, 6:17 p.m. UTC | #2 
Ludovic Courtès <ludo@gnu.org> writes:

> Hi,
>
> Christopher Baines <mail@cbaines.net> skribis:
>
>> I think there are a couple of potential uses for this. It's somewhat a
>> separation in to what checkers are just checking the contents of the
>> repository (line length for example), and other checkers which are bringing in
>> external information which could change.
>>
>> I'm thinking particularly, about treating network dependant checkers
>> differently when automatically running them, but this commit also adds a
>> --no-network flag to guix lint, which selects the checkers that don't access
>> the network, which could be useful if no network access is available.
>>
>> * guix/lint.scm (%checkers): Rename to %all-checkers.
>> (%local-checkers, %network-dependant-checkers): New variables.
>> * guix/scripts/lint.scm (run-checkers): Make the checkers argument mandatory.
>> (list-checkers-and-exit): Handle the checkers as an argument.
>> (%options): Adjust for changes to %checkers, add a --no-network option, and
>> change how the --list-checkers option is handled.
>> (guix-lint): Adjust indentation, and update how the checkers are handled.
>
> Nice.
>
>> +(define %network-dependant-checkers
>                           ^
> Shouldn’t it be “dependent” with an ‘e’?

I'm definitely not an authority on spelling, but yeah, it seems like
dependent is preferred as the adjective, especially in American English.

> Otherwise LGTM, thanks!

Great :)
diff mbox series

Patch

diff --git a/guix/lint.scm b/guix/lint.scm
index f86e494be5..2cc0d34440 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -91,7 +91,10 @@ 
 
             emit-warnings
 
-            %checkers
+            %local-checkers
+            %network-dependant-checkers
+            %all-checkers
+
             lint-checker
             lint-checker?
             lint-checker-name
@@ -1158,16 +1161,12 @@  them for PACKAGE."
 ;;; List of checkers.
 ;;;
 
-(define %checkers
+(define %local-checkers
   (list
    (lint-checker
      (name        'description)
      (description "Validate package descriptions")
      (check       check-description-style))
-   (lint-checker
-     (name        'gnu-description)
-     (description "Validate synopsis & description of GNU packages")
-     (check       check-gnu-synopsis+description))
    (lint-checker
      (name        'inputs-should-be-native)
      (description "Identify inputs that should be native inputs")
@@ -1176,14 +1175,6 @@  them for PACKAGE."
      (name        'inputs-should-not-be-input)
      (description "Identify inputs that shouldn't be inputs at all")
      (check       check-inputs-should-not-be-an-input-at-all))
-   (lint-checker
-     (name        'patch-file-names)
-     (description "Validate file names and availability of patches")
-     (check       check-patch-file-names))
-   (lint-checker
-     (name        'home-page)
-     (description "Validate home-page URLs")
-     (check       check-home-page))
    (lint-checker
      (name        'license)
      ;; TRANSLATORS: <license> is the name of a data type and must not be
@@ -1191,18 +1182,10 @@  them for PACKAGE."
      (description "Make sure the 'license' field is a <license> \
 or a list thereof")
      (check       check-license))
-   (lint-checker
-     (name        'source)
-     (description "Validate source URLs")
-     (check       check-source))
    (lint-checker
      (name        'mirror-url)
      (description "Suggest 'mirror://' URLs")
      (check       check-mirror-url))
-   (lint-checker
-     (name        'github-url)
-     (description "Suggest GitHub URLs")
-     (check       check-github-url))
    (lint-checker
      (name        'source-file-name)
      (description "Validate file names of sources")
@@ -1215,10 +1198,37 @@  or a list thereof")
      (name        'derivation)
      (description "Report failure to compile a package to a derivation")
      (check       check-derivation))
+   (lint-checker
+    (name        'patch-file-names)
+    (description "Validate file names and availability of patches")
+    (check       check-patch-file-names))
+   (lint-checker
+     (name        'formatting)
+     (description "Look for formatting issues in the source")
+     (check       check-formatting))))
+
+(define %network-dependant-checkers
+  (list
    (lint-checker
      (name        'synopsis)
      (description "Validate package synopses")
      (check       check-synopsis-style))
+   (lint-checker
+     (name        'gnu-description)
+     (description "Validate synopsis & description of GNU packages")
+     (check       check-gnu-synopsis+description))
+   (lint-checker
+     (name        'home-page)
+     (description "Validate home-page URLs")
+     (check       check-home-page))
+   (lint-checker
+     (name        'source)
+     (description "Validate source URLs")
+     (check       check-source))
+   (lint-checker
+     (name        'github-url)
+     (description "Suggest GitHub URLs")
+     (check       check-github-url))
    (lint-checker
      (name        'cve)
      (description "Check the Common Vulnerabilities and Exposures\
@@ -1227,8 +1237,8 @@  or a list thereof")
    (lint-checker
      (name        'refresh)
      (description "Check the package for new upstream releases")
-     (check       check-for-updates))
-   (lint-checker
-     (name        'formatting)
-     (description "Look for formatting issues in the source")
-     (check       check-formatting))))
+     (check       check-for-updates))))
+
+(define %all-checkers
+  (append %local-checkers
+          %network-dependant-checkers))
diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index 8a8ffc8f28..c2e022cf94 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -38,7 +38,7 @@ 
   #:export (guix-lint
             run-checkers))
 
-(define* (run-checkers package #:optional (checkers %checkers))
+(define (run-checkers package checkers)
   "Run the given CHECKERS on PACKAGE."
   (let ((tty? (isatty? (current-error-port))))
     (for-each (lambda (checker)
@@ -54,14 +54,14 @@ 
       (format (current-error-port) "\x1b[K")
       (force-output (current-error-port)))))
 
-(define (list-checkers-and-exit)
+(define (list-checkers-and-exit checkers)
   ;; Print information about all available checkers and exit.
   (format #t (G_ "Available checkers:~%"))
   (for-each (lambda (checker)
               (format #t "- ~a: ~a~%"
                       (lint-checker-name checker)
                       (G_ (lint-checker-description checker))))
-            %checkers)
+            checkers)
   (exit 0))
 
 
@@ -97,26 +97,33 @@  run the checkers on all packages.\n"))
   ;;                                  'certainty'.
   (list (option '(#\c "checkers") #t #f
                 (lambda (opt name arg result)
-                  (let ((names (map string->symbol (string-split arg #\,))))
+                  (let ((names (map string->symbol (string-split arg #\,)))
+                        (checker-names (map lint-checker-name %all-checkers)))
                     (for-each (lambda (c)
-                                (unless (memq c
-                                              (map lint-checker-name
-                                                   %checkers))
+                                (unless (memq c checker-names)
                                   (leave (G_ "~a: invalid checker~%") c)))
                               names)
                     (alist-cons 'checkers
                                 (filter (lambda (checker)
                                           (member (lint-checker-name checker)
                                                   names))
-                                        %checkers)
+                                        %all-checkers)
                                 result))))
+        (option '(#\n "no-network") #f #f
+                (lambda (opt name arg result)
+                  (alist-cons 'checkers
+                              %local-checkers
+                              (alist-delete 'checkers
+                                            result))))
         (option '(#\h "help") #f #f
                 (lambda args
                   (show-help)
                   (exit 0)))
         (option '(#\l "list-checkers") #f #f
-                (lambda args
-                   (list-checkers-and-exit)))
+                (lambda (opt name arg result)
+                  (alist-cons 'list?
+                              #t
+                              result)))
         (option '(#\V "version") #f #f
                 (lambda args
                   (show-version-and-exit "guix lint")))))
@@ -134,13 +141,17 @@  run the checkers on all packages.\n"))
 
   (let* ((opts (parse-options))
          (args (filter-map (match-lambda
-                            (('argument . value)
-                             value)
-                            (_ #f))
+                             (('argument . value)
+                              value)
+                             (_ #f))
                            (reverse opts)))
-         (checkers (or (assoc-ref opts 'checkers) %checkers)))
-     (if (null? args)
-          (fold-packages (lambda (p r) (run-checkers p checkers)) '())
-          (for-each (lambda (spec)
-                      (run-checkers (specification->package spec) checkers))
-                    args))))
+         (checkers (or (assoc-ref opts 'checkers) %all-checkers)))
+    (cond
+     ((assoc-ref opts 'list?)
+      (list-checkers-and-exit checkers))
+     ((null? args)
+      (fold-packages (lambda (p r) (run-checkers p checkers)) '()))
+     (else
+      (for-each (lambda (spec)
+                  (run-checkers (specification->package spec) checkers))
+                args)))))