| Message ID | 87a6acjncd.fsf_-_@gnu.org |
|---|---|
| State | Accepted |
| Headers | show |
Hi Ludovic, > Applied with the cosmetic changes below, mostly aiming to visually > simplify the code and make it consistent with the rest. Thank you for the correction and application! > It’s great that you went to great lengths to implement tests for this, > as Maxime had suggested. To me, the complexity of a test must be > justified by its “bug-finding performance”; in this particular case, I > think we’re borderline: the tests are a little bit complex and unlikely > to find new bugs. > > Thanks for all the work and for your feedback on your experience > programming with Guile! I only discovered the problem, I was able to implement it mostly thanks to Maxime and Tobias! The code review experience was so good that I even posted the following :) https://fosstodon.org/web/@taiju/108458633893022791 https://fosstodon.org/web/@taiju/108458643302758263 I'd like to know for future contributions. I like functional programming and I love compose, (ice-9 curried-definitions), and SRFI 26 in my programs, but should I use them less in the code I put in Guix? Thanks, -- Taiju
Hello, Taiju HIGASHI <higashi@taiju.info> skribis: > I only discovered the problem, I was able to implement it mostly thanks > to Maxime and Tobias! > The code review experience was so good that I even posted the following > :) > https://fosstodon.org/web/@taiju/108458633893022791 > https://fosstodon.org/web/@taiju/108458643302758263 Heh, good to know that it was a positive experience! > I'd like to know for future contributions. > I like functional programming and I love compose, (ice-9 > curried-definitions), and SRFI 26 in my programs, but should I use them > less in the code I put in Guix? Probably. It’s tempting to use these if you come with a Haskell background, say. But in some cases, they make things less readable; that’s the case with the way ‘make-dummy-file’ was written IMO. Guix code uses SRFI-26 in some places; I think (ice-9 curried-definitions) is not used anywhere but I think it’s fine to use it if it helps. Thanks, Ludo’.
Hi, >> I'd like to know for future contributions. >> I like functional programming and I love compose, (ice-9 >> curried-definitions), and SRFI 26 in my programs, but should I use them >> less in the code I put in Guix? > > Probably. It’s tempting to use these if you come with a Haskell > background, say. But in some cases, they make things less readable; > that’s the case with the way ‘make-dummy-file’ was written IMO. > > Guix code uses SRFI-26 in some places; I think (ice-9 > curried-definitions) is not used anywhere but I think it’s fine to use > it if it helps. Thank you for your answers! I like Haskell and OCaml, but I don't have experience with them much. I like Scheme and Common Lisp more :) The point-free style is beautiful, but it is often just self-satisfaction, so I think after hearing your opinion that better use them less in shared code. However, those features that make it possible are very good, and I will continue to use them in the programs I write for personal use :) Thanks,
Hi! Taiju HIGASHI <higashi@taiju.info> skribis: > The point-free style is beautiful, but it is often just > self-satisfaction, so I think after hearing your opinion that better use > them less in shared code. On this topic and others, the manual links to Riastradh’s style guide for Scheme (info "(guix) Formatting Code"), which is worth a read! Ludo’.
>> The point-free style is beautiful, but it is often just >> self-satisfaction, so I think after hearing your opinion that better use >> them less in shared code. > > On this topic and others, the manual links to Riastradh’s style guide > for Scheme (info "(guix) Formatting Code"), which is worth a read! Thanks for sharing the good information! (I forgot to CC my reply, so I resent it.)
diff --git a/guix/ui.scm b/guix/ui.scm index 93707a7a4b..a7acd41440 100644 --- a/guix/ui.scm +++ b/guix/ui.scm @@ -1674,15 +1674,13 @@ (define* (pager-wrapped-port #:optional (port (current-output-port))) #f))) (define (find-available-pager) - "Returns the program name or path of an available pager. -If neither less nor more is installed, return an empty string so that -call-with-paginated-output-port will not call pager." + "Return the program name of an available pager or the empty string if none is +available." (or (getenv "GUIX_PAGER") (getenv "PAGER") (which "less") (which "more") - "" ;; Returns an empty string so that call-with-paginated-output-port does not call pager. - )) + "")) (define* (call-with-paginated-output-port proc #:key (less-options "FrX")) diff --git a/tests/ui.scm b/tests/ui.scm index ff83e66a7e..6a25a204ca 100644 --- a/tests/ui.scm +++ b/tests/ui.scm @@ -294,13 +294,12 @@ (define guile-2.0.9 (>0 (package-relevance libb2 (map rx '("crypto" "library"))))))) -(define make-dummy-file - (compose - close-port - open-output-file - (cut in-vicinity <> <>))) +(define (make-empty-file directory file) + ;; Create FILE in DIRECTORY. + (close-port (open-output-file (in-vicinity directory file)))) (define (assert-equals-find-available-pager expected) + ;; Use 'with-paginated-output-port' and return true if it invoked EXPECTED. (define used-command "") (mock ((ice-9 popen) open-pipe* (lambda (mode command . args) @@ -314,56 +313,51 @@ (define used-command "") (string=? expected used-command))))) -(test-assert "find-available-pager, All environment variables are specified and both less and more are installed" +(test-assert "find-available-pager, GUIX_PAGER takes precedence" (call-with-temporary-directory (lambda (dir) - (with-environment-variables - `(("PATH" ,dir) - ("GUIX_PAGER" "guix-pager") - ("PAGER" "pager")) - (make-dummy-file dir "less") - (make-dummy-file dir "more") + (with-environment-variables `(("PATH" ,dir) + ("GUIX_PAGER" "guix-pager") + ("PAGER" "pager")) + (make-empty-file dir "less") + (make-empty-file dir "more") (assert-equals-find-available-pager "guix-pager"))))) -(test-assert "find-available-pager, GUIX_PAGER is not specified" +(test-assert "find-available-pager, PAGER takes precedence" (call-with-temporary-directory (lambda (dir) - (with-environment-variables - `(("PATH" ,dir) - ("GUIX_PAGER" #false) - ("PAGER" "pager")) - (make-dummy-file dir "less") - (make-dummy-file dir "more") + (with-environment-variables `(("PATH" ,dir) + ("GUIX_PAGER" #false) + ("PAGER" "pager")) + (make-empty-file dir "less") + (make-empty-file dir "more") (assert-equals-find-available-pager "pager"))))) -(test-assert "find-available-pager, All environment variables are not specified and both less and more are installed" +(test-assert "find-available-pager, 'less' takes precedence" (call-with-temporary-directory (lambda (dir) - (with-environment-variables - `(("PATH" ,dir) - ("GUIX_PAGER" #false) - ("PAGER" #false)) - (make-dummy-file dir "less") - (make-dummy-file dir "more") + (with-environment-variables `(("PATH" ,dir) + ("GUIX_PAGER" #false) + ("PAGER" #false)) + (make-empty-file dir "less") + (make-empty-file dir "more") (assert-equals-find-available-pager (in-vicinity dir "less")))))) -(test-assert "find-available-pager, All environment variables are not specified and more is installed" +(test-assert "find-available-pager, 'more' takes precedence" (call-with-temporary-directory (lambda (dir) - (with-environment-variables - `(("PATH" ,dir) - ("GUIX_PAGER" #false) - ("PAGER" #false)) - (make-dummy-file dir "more") + (with-environment-variables `(("PATH" ,dir) + ("GUIX_PAGER" #false) + ("PAGER" #false)) + (make-empty-file dir "more") (assert-equals-find-available-pager (in-vicinity dir "more")))))) -(test-assert "find-available-pager, All environment variables are not specified and both less and more are not installed" +(test-assert "find-available-pager, no pager" (call-with-temporary-directory (lambda (dir) - (with-environment-variables - `(("PATH" ,dir) - ("GUIX_PAGER" #false) - ("PAGER" #false)) + (with-environment-variables `(("PATH" ,dir) + ("GUIX_PAGER" #false) + ("PAGER" #false)) (assert-equals-find-available-pager ""))))) (test-end "ui")