| Message ID | 20191202210127.28466-1-zimon.toutoune@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | lint: Add '--load-path' option. | expand |
Hi! zimoun <zimon.toutoune@gmail.com> skribis: > * guix/scripts/lint.scm (%options): Add '--load-path' option. > * tests/guix-lint.sh: Test it. Awesome, I’ve been wanting that for a long time. :-) > +LOAD_PATH="$module_dir" > + > +out=`guix lint -L $LOAD_PATH -c synopsis,description dummy 2>&1` Perhaps you don’t even need to define ‘LOAD_PATH’? Could you add ‘-L’ to guix.texi, and then I guess we’re done! Thanks, Ludo’.
Hi, On Wed, 4 Dec 2019 at 18:10, Ludovic Courtès <ludo@gnu.org> wrote: > zimoun <zimon.toutoune@gmail.com> skribis: > > > * guix/scripts/lint.scm (%options): Add '--load-path' option. > > * tests/guix-lint.sh: Test it. > > Awesome, I’ve been wanting that for a long time. :-) But I find this "hacky". Because it is copy/paste from other --load-path option elsewhere (probably "guix show" :-)). It should be refactored to have only one function and call it where the command needs it. I mean, the same way that 'show-build-options-help' or '(append %transformation-options %standard-build-options)' does in "guix package". Other said, %standard-build-options should be splited and go to say misc.scm or common.scm or whatever -- I do not know enough the file organization. :-) Now, the same --load-path code at 3 different places. Bad practise... IMHO. Plus, we will win more flexibility to write more commands. ;-) What do you think? > > +LOAD_PATH="$module_dir" > > + > > +out=`guix lint -L $LOAD_PATH -c synopsis,description dummy 2>&1` > > Perhaps you don’t even need to define ‘LOAD_PATH’? Done. > Could you add ‘-L’ to guix.texi, and then I guess we’re done! I did. My bad. Cheers, simon
Hi! zimoun <zimon.toutoune@gmail.com> skribis: > On Wed, 4 Dec 2019 at 18:10, Ludovic Courtès <ludo@gnu.org> wrote: > >> zimoun <zimon.toutoune@gmail.com> skribis: >> >> > * guix/scripts/lint.scm (%options): Add '--load-path' option. >> > * tests/guix-lint.sh: Test it. >> >> Awesome, I’ve been wanting that for a long time. :-) > > But I find this "hacky". Because it is copy/paste from other > --load-path option elsewhere (probably "guix show" :-)). Well, technically speaking it’s not copy/pasted since the code you sent uses ‘find’ to grab the option; plus, we’re talking about very few lines, which is typically hard to factorize. So I sympathize with the bad feeling of repetition, but I’m not quite sure how this can be avoided in this case. WDYT? Thanks, Ludo’.
Hi Ludo, On Sat, 7 Dec 2019 at 23:51, Ludovic Courtès <ludo@gnu.org> wrote: > Well, technically speaking it’s not copy/pasted since the code you sent > uses ‘find’ to grab the option; plus, we’re talking about very few > lines, which is typically hard to factorize. I agree. Even if I do not have a better solution to propose, I just feel the current one is not optimal. Does the same gettext entries G_ are duplicated? Do translators translate 2 times (or more) the same string? > So I sympathize with the bad feeling of repetition, but I’m not quite > sure how this can be avoided in this case. > > WDYT? Thank you for the explanations. I am not sure neither and need to fail by myself to be convinced. ;-) And currently, any hypothetical break should be reported by the test suite. Hope so. All the best, simon
Hi, zimoun <zimon.toutoune@gmail.com> skribis: > On Sat, 7 Dec 2019 at 23:51, Ludovic Courtès <ludo@gnu.org> wrote: > >> Well, technically speaking it’s not copy/pasted since the code you sent >> uses ‘find’ to grab the option; plus, we’re talking about very few >> lines, which is typically hard to factorize. > > I agree. Even if I do not have a better solution to propose, I just > feel the current one is not optimal. > > Does the same gettext entries G_ are duplicated? Do translators > translate 2 times (or more) the same string? No, they’re deduplicated. >> So I sympathize with the bad feeling of repetition, but I’m not quite >> sure how this can be avoided in this case. >> >> WDYT? > > Thank you for the explanations. I am not sure neither and need to fail > by myself to be convinced. ;-) > And currently, any hypothetical break should be reported by the test > suite. Hope so. I think so. I’ve applied it now, thank you! Ludo’.
diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm index 1668d02992..8d08c484f5 100644 --- a/guix/scripts/lint.scm +++ b/guix/scripts/lint.scm @@ -9,6 +9,7 @@ ;;; Copyright © 2017 Tobias Geerinckx-Rice <me@tobias.gr> ;;; Copyright © 2017, 2018 Efraim Flashner <efraim@flashner.co.il> ;;; Copyright © 2018, 2019 Arun Isaac <arunisaac@systemreboot.net> +;;; Copyright © 2019 Simon Tournier <zimon.toutoune@gmail.com> ;;; ;;; This file is part of GNU Guix. ;;; @@ -30,6 +31,7 @@ #:use-module (guix lint) #:use-module (guix ui) #:use-module (guix scripts) + #:use-module (guix scripts build) #:use-module (gnu packages) #:use-module (ice-9 match) #:use-module (ice-9 format) @@ -94,6 +96,9 @@ run the checkers on all packages.\n")) -c, --checkers=CHECKER1,CHECKER2... only run the specified checkers")) (display (G_ " + -L, --load-path=DIR prepend DIR to the package module search path")) + (newline) + (display (G_ " -h, --help display this help and exit")) (display (G_ " -l, --list-checkers display the list of available lint checkers")) @@ -128,6 +133,9 @@ run the checkers on all packages.\n")) %local-checkers (alist-delete 'checkers result)))) + (find (lambda (option) + (member "load-path" (option-names option))) + %standard-build-options) (option '(#\h "help") #f #f (lambda args (show-help) diff --git a/tests/guix-lint.sh b/tests/guix-lint.sh index 7ddc7c265b..f07ccb4e1a 100644 --- a/tests/guix-lint.sh +++ b/tests/guix-lint.sh @@ -76,3 +76,15 @@ then true; else false; fi # Make sure specifying multiple packages works. guix lint -c inputs-should-be-native dummy dummy@42 dummy + + +# Use --load-path instead. +unset GUIX_PACKAGE_PATH +LOAD_PATH="$module_dir" + +out=`guix lint -L $LOAD_PATH -c synopsis,description dummy 2>&1` +if [ `grep_warning "$out"` -ne 3 ] +then false; else true; fi + +# Make sure specifying multiple packages works. +guix lint -L $LOAD_PATH -c inputs-should-be-native dummy dummy@42 dummy