| Message ID | 20220608102257.15042-1-higashi@taiju.info |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Improve pager selection logic when less is not installed | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
Taiju HIGASHI schreef op wo 08-06-2022 om 19:22 [+0900]: > +(define available-pager > + (if (which "less") > + "less" > + (if (which "more") > + "more" > + #f))) Can be simplified to something like,: (define (find-available-pager) "[appropriate docstring]" (or (getenv "GUIX_PAGER") ;; <-- simplify 'if' chains by using 'or' (getenv "PAGER") (which "less") (which "more") ;; <--- TODO: how to handle no pager being found? )) and (let ((pager-command-line (available-pager))) [...]) I've thunked find-available-pager here, such that call-with-paginated- output-port respects the $PATH that is set before call-with-paginated- output-port instead of the $PATH from when "guix ui" was loaded? Ideally there would be some regression tests as well. Greetings, Maxime
[Please keep debbugs in CC] Taiju HIGASHI schreef op wo 08-06-2022 om 22:33 [+0900]: > > Ideally there would be some regression tests as well. > > I think I can write tests if I can figure out how to give a minimal > specific package to the test preconditions, do you have any test > codes > that are similar and can be used as a reference? Not really, but FWIW it might be convenient to use the with-environment-variables macro and mock the call to open-pipe* with the 'mock' macro to make sure that open-pipe* is called with the ‘correct’ pager according to PATH (*). Searching for 'mock' with "git grep -F" should find some examples. (*) call-with-temporary-directory + chmod + call-with-output-file may be useful for setting up a simulated $PATH with a dummy 'less' and/or 'more'. Greetings, Maxime.
Maxime Devos <maximedevos@telenet.be> writes: > [Please keep debbugs in CC] I'm sorry. > Taiju HIGASHI schreef op wo 08-06-2022 om 22:33 [+0900]: >> > Ideally there would be some regression tests as well. >> >> I think I can write tests if I can figure out how to give a minimal >> specific package to the test preconditions, do you have any test >> codes >> that are similar and can be used as a reference? > > Not really, but FWIW it might be convenient to use the > with-environment-variables macro and mock the call to open-pipe* with > the 'mock' macro to make sure that open-pipe* is called with the > ‘correct’ pager according to PATH (*). Searching for 'mock' with "git > grep -F" should find some examples. > > (*) call-with-temporary-directory + chmod + call-with-output-file may > be useful for setting up a simulated $PATH with a dummy 'less' and/or > 'more'. Thanks for the implementation tips. I will try to implement the test as well, although it will be after tomorrow. Is tests/ui.scm the right place to implement the tests? Thanks,
Taiju HIGASHI schreef op do 09-06-2022 om 00:17 [+0900]: > Thanks for the implementation tips. I will try to implement the test as > well, although it will be after tomorrow. Is tests/ui.scm the right > place to implement the tests? Looks like the right location to me ,yes. Greetings, Maxime.
diff --git a/guix/ui.scm b/guix/ui.scm index cb68a07c6c..22169a7eb8 100644 --- a/guix/ui.scm +++ b/guix/ui.scm @@ -17,6 +17,7 @@ ;;; Copyright © 2020 Arun Isaac <arunisaac@systemreboot.net> ;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com> ;;; Copyright © 2018 Steve Sprang <scs@stevesprang.com> +;;; Copyright © 2022 Taiju HIGASHI <higashi@taiju.info> ;;; ;;; This file is part of GNU Guix. ;;; @@ -1672,11 +1673,18 @@ (define* (pager-wrapped-port #:optional (port (current-output-port))) (_ #f))) +(define available-pager + (if (which "less") + "less" + (if (which "more") + "more" + #f))) + (define* (call-with-paginated-output-port proc #:key (less-options "FrX")) (let ((pager-command-line (or (getenv "GUIX_PAGER") (getenv "PAGER") - "less"))) + available-pager))) ;; Setting PAGER to the empty string conventionally disables paging. (if (and (not (string-null? pager-command-line)) (isatty?* (current-output-port)))