GNU bug report logs - #55845
[PATCH 0/1] Improve pager selection logic when less is not installed

Previous Next

Package: guix-patches;

Reported by: Taiju HIGASHI <higashi <at> taiju.info>

Date: Wed, 8 Jun 2022 10:22:02 UTC

Severity: normal

Tags: patch

Done: Ludovic Courtès <ludo <at> gnu.org>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 55845 in the body.
You can then email your comments to 55845 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Wed, 08 Jun 2022 10:22:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Taiju HIGASHI <higashi <at> taiju.info>:
New bug report received and forwarded. Copy sent to guix-patches <at> gnu.org. (Wed, 08 Jun 2022 10:22:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Taiju HIGASHI <higashi <at> taiju.info>
To: guix-patches <at> gnu.org
Cc: Taiju HIGASHI <higashi <at> taiju.info>
Subject: [PATCH 0/1] Improve pager selection logic when less is not installed
Date: Wed,  8 Jun 2022 19:21:24 +0900
Hi,

The problem rarely occurs, but when we run guix commands in an environment
where "less" is not installed we get an error.

This is the same problem reported at the following URL
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1012405

If "more" could be specified as an alternative program to "less", the problem
would be less likely to occur at least in a POSIX environment. Also, I would
like to avoid using the pager in special environments where "more" is not
installed at all.

I have written a patch to solve the above.

I am concerned about performance degradation due to more unnecessary
processing.
If you have another good solution, please let me know.
Also, if you feel that this is a minor issue and not worth addressing, please
feel free to dismiss it. (Still, a fix to make the error message more friendly
might be a good idea.)

Best Regards,

Taiju HIGASHI (1):
  ui: Improve pager selection logic when less is not installed.

 guix/ui.scm | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

--
2.36.1




Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Wed, 08 Jun 2022 10:24:01 GMT) Full text and rfc822 format available.

Message #8 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Taiju HIGASHI <higashi <at> taiju.info>
To: 55845 <at> debbugs.gnu.org
Cc: Taiju HIGASHI <higashi <at> taiju.info>
Subject: [PATCH 1/1] ui: Improve pager selection logic when less is not
 installed.
Date: Wed,  8 Jun 2022 19:22:57 +0900
* guix/ui.scm (available-pager): New variable. Holds available pagers.
  (call-with-paginated-output-port): Get an alternative program from the
  available-pager variable when the environment variable is not set.
---
 guix/ui.scm | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

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 <at> systemreboot.net>
 ;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
 ;;; Copyright © 2018 Steve Sprang <scs <at> stevesprang.com>
+;;; Copyright © 2022 Taiju HIGASHI <higashi <at> 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)))
-- 
2.36.1





Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Wed, 08 Jun 2022 12:52:01 GMT) Full text and rfc822 format available.

Message #11 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: Taiju HIGASHI <higashi <at> taiju.info>
Cc: 55845 <at> debbugs.gnu.org, guix-patches <at> gnu.org
Subject: Re: [bug#55845] [PATCH 0/1] Improve pager selection logic when less
 is not installed
Date: Wed, 08 Jun 2022 14:14:34 +0200
[Message part 1 (text/plain, inline)]
Hi!

Taiju HIGASHI 写道:
> The problem rarely occurs, but when we run guix commands in an 
> environment
> where "less" is not installed we get an error.

True.  Odd that it's gone unreported(?) for so long.

> I am concerned about performance degradation due to more 
> unnecessary
> processing.

Since you asked…  :-)

One way that this is ‘expensive’ is that it always calls WHICH at 
least once, no matter what Guix was invoked to do.

If you're familiar with Haskell or Nix: Scheme is not that, it's 
not ‘lazy’ and will evaluate the (if (which "less") …) even when 
the value is never used.  Turning AVAILABLE-PAGER into a procedure 
would avoid that.

Also, you're looking up the final pager in $PATH twice: you call 
WHICH, but then discard its work by returning the relative string 
"less".

The final OPEN-PIPE* invokes a shell which will search $PATH 
again.  We could save it the trouble by returning an absolute file 
name: the result of WHICH.

And since WHICH returns #f on failure, you can replace the nested 
IFs with a single OR:

 (define (available-pager)
   (or (which "less")
       (which "more")))

And well, as you probably noticed by now, it's actually more clear 
and concise if we just in-line what little is left:

 (let ((pager-command-line (or (getenv "GUIX_PAGER")
                               (getenv "PAGER")
                               (which "less")
                               (which "more")
                               "")))
   …

Your original patch returns #f if no pages could be found.  I 
don't think that is handled, but "" is, so return that instead.

Now I think that's 100% equivalent to your original; let me know 
if I missed a spot.

> Also, if you feel that this is a minor issue and not worth 
> addressing, please
> feel free to dismiss it. (Still, a fix to make the error message 
> more friendly
> might be a good idea.)

It *is* minor, but then so is the fix, and as written above it 
doesn't add ‘overhead’.  I think it's a good idea to check for 
"more" (but no more) and silently disable paging otherwise.

Thanks!

T G-R
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Wed, 08 Jun 2022 12:52:02 GMT) Full text and rfc822 format available.

Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Wed, 08 Jun 2022 13:13:01 GMT) Full text and rfc822 format available.

Message #17 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Taiju HIGASHI <higashi <at> taiju.info>
To: Tobias Geerinckx-Rice <me <at> tobias.gr>
Cc: 55845 <at> debbugs.gnu.org, guix-patches <at> gnu.org
Subject: Re: [bug#55845] [PATCH 0/1] Improve pager selection logic when less
 is not installed
Date: Wed, 08 Jun 2022 22:12:34 +0900
Hi 写道,

Thank you for reviwing!

> Hi!
>
> Taiju HIGASHI 写道:
>> The problem rarely occurs, but when we run guix commands in an
>> environment
>> where "less" is not installed we get an error.
>
> True.  Odd that it's gone unreported(?) for so long.
>
>> I am concerned about performance degradation due to more unnecessary
>> processing.
>
> Since you asked…  :-)
>
> One way that this is ‘expensive’ is that it always calls WHICH at
> least once, no matter what Guix was invoked to do.
>
> If you're familiar with Haskell or Nix: Scheme is not that, it's not
> ‘lazy’ and will evaluate the (if (which "less") …) even when the
> value is never used.  Turning AVAILABLE-PAGER into a procedure would
> avoid that.

I understand that I can delay the evaluation timing if I make it a
procedure, but is my understanding correct that the number of calls will
remain the same because it will be evaluated each time the
`call-with-paginated-output-port` procedure is called?

I agree with your point that it would be better to make it a procedure,
as it would be more eco-friendly to not have to evaluate when GUIX_PAGER
or PAGER is specified.

> Also, you're looking up the final pager in $PATH twice: you call
> WHICH, but then discard its work by returning the relative string
> "less".
>
> The final OPEN-PIPE* invokes a shell which will search $PATH again.
> We could save it the trouble by returning an absolute file name: the
> result of WHICH.

I see, I did not understand that behavior. Thank you.

> And since WHICH returns #f on failure, you can replace the nested IFs
> with a single OR:
>
>  (define (available-pager)
>    (or (which "less")
>        (which "more")))

This one is also more readable. Thank you.

> And well, as you probably noticed by now, it's actually more clear and
> concise if we just in-line what little is left:
>
>  (let ((pager-command-line (or (getenv "GUIX_PAGER")
>                                (getenv "PAGER")
>                                (which "less")
>                                (which "more")
>                                "")))
>    …

You mean that the $PATH lookup in open-pipe can be suppressed?
Also, I misunderstood the string-null? spec; available-pager should have
returned an empty string.

> Your original patch returns #f if no pages could be found.  I don't
> think that is handled, but "" is, so return that instead.
>
> Now I think that's 100% equivalent to your original; let me know if I
> missed a spot.

I thought what you said was completely correct.

>> Also, if you feel that this is a minor issue and not worth
>> addressing, please
>> feel free to dismiss it. (Still, a fix to make the error message
>> more friendly
>> might be a good idea.)
>
> It *is* minor, but then so is the fix, and as written above it doesn't
> add ‘overhead’.  I think it's a good idea to check for "more" (but
> no more) and silently disable paging otherwise.

I will just write what you have told me, but may I continue to modify
the patch?

Thank,
--
Taiju




Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Wed, 08 Jun 2022 13:13:02 GMT) Full text and rfc822 format available.

Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Wed, 08 Jun 2022 13:19:02 GMT) Full text and rfc822 format available.

Message #23 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Taiju HIGASHI <higashi <at> taiju.info>, 55845 <at> debbugs.gnu.org
Subject: Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when
 less is not installed.
Date: Wed, 08 Jun 2022 15:18:20 +0200
[Message part 1 (text/plain, inline)]
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
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Wed, 08 Jun 2022 14:40:02 GMT) Full text and rfc822 format available.

Message #26 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: Taiju HIGASHI <higashi <at> taiju.info>
Cc: 55845 <at> debbugs.gnu.org, guix-patches <at> gnu.org
Subject: Re: [bug#55845] [PATCH 0/1] Improve pager selection logic when less
 is not installed
Date: Wed, 08 Jun 2022 16:22:08 +0200
[Message part 1 (text/plain, inline)]
Hi again,

Taiju HIGASHI 写道:
> I understand that I can delay the evaluation timing if I make it 
> a
> procedure, but is my understanding correct that the number of 
> calls will
> remain the same because it will be evaluated each time the
> `call-with-paginated-output-port` procedure is called?

Previously, it would have been evaluated even if 
call-with-paginated-output-port was never called at all.

As for the >0 calls case: yes… but when do we expect 
call-with-paginated-output-port to be called more than once per 
run?

The use case for this code is to do something, then display it in 
a pager and exit.  I think calling it multiple times in one run 
would imply bad UX.

Do I misunderstand?

> I agree with your point that it would be better to make it a 
> procedure,
> as it would be more eco-friendly to not have to evaluate when 
> GUIX_PAGER
> or PAGER is specified.

I wish the rest of Guix were so efficient that it mattered :-)

[/me is waiting for ‘guix pull’ as I reply to multiple mails, on 
battery…]

Regardless, not calling a procedure at all is even more efficient 
and IMO more readable here.

> You mean that the $PATH lookup in open-pipe can be suppressed?

Yes.  OPEN-PIPE* won't need to stat $PATH at all if we give it 
"/run/current-system/profile/bin/less" instead of "less".

(It's not relevant to the above, but my previously reply 
mistakenly mentioned a shell — there is no shell involved with 
OPEN-PIPE*, only with OPEN-PIPE.  Sorry.)

> I will just write what you have told me, but may I continue to 
> modify
> the patch?

Of course!  Curious how, though.

Kind regards,

T G-R
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Wed, 08 Jun 2022 14:40:02 GMT) Full text and rfc822 format available.

Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Wed, 08 Jun 2022 15:09:01 GMT) Full text and rfc822 format available.

Message #32 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Taiju HIGASHI <higashi <at> taiju.info>
Cc: 55845 <at> debbugs.gnu.org
Subject: Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when
 less is not installed.
Date: Wed, 08 Jun 2022 17:08:13 +0200
[Message part 1 (text/plain, inline)]
[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.
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Wed, 08 Jun 2022 15:10:02 GMT) Full text and rfc822 format available.

Message #35 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Taiju HIGASHI <higashi <at> taiju.info>
To: Tobias Geerinckx-Rice <me <at> tobias.gr>
Cc: 55845 <at> debbugs.gnu.org, guix-patches <at> gnu.org
Subject: Re: [bug#55845] [PATCH 0/1] Improve pager selection logic when less
 is not installed
Date: Thu, 09 Jun 2022 00:09:16 +0900
Hi Tobias,

Thank you kindly for your detailed explanation.

Tobias Geerinckx-Rice <me <at> tobias.gr> writes:

> Hi again,
>
> Taiju HIGASHI 写道:
>> I understand that I can delay the evaluation timing if I make it a
>> procedure, but is my understanding correct that the number of calls
>> will
>> remain the same because it will be evaluated each time the
>> `call-with-paginated-output-port` procedure is called?
>
> Previously, it would have been evaluated even if
> call-with-paginated-output-port was never called at all.
>
> As for the >0 calls case: yes… but when do we expect
> call-with-paginated-output-port to be called more than once per run?
>
> The use case for this code is to do something, then display it in a
> pager and exit.  I think calling it multiple times in one run would
> imply bad UX.
>
> Do I misunderstand?

No, you don't.
As you said, my implementation was a bad idea, as
call-with-paginated-output-port is executed even when it is not needed.
It seems unlikely that call-with-paginated-output-port will be called
more than once in a single process. I did not have enough insight.

>> I agree with your point that it would be better to make it a
>> procedure,
>> as it would be more eco-friendly to not have to evaluate when
>> GUIX_PAGER
>> or PAGER is specified.
>
> I wish the rest of Guix were so efficient that it mattered :-)
>
> [/me is waiting for ‘guix pull’ as I reply to multiple mails, on
> battery…]
>
> Regardless, not calling a procedure at all is even more efficient and
> IMO more readable here.

I agree.

>> You mean that the $PATH lookup in open-pipe can be suppressed?
>
> Yes.  OPEN-PIPE* won't need to stat $PATH at all if we give it
> "/run/current-system/profile/bin/less" instead of "less".
>
> (It's not relevant to the above, but my previously reply mistakenly
> mentioned a shell ― there is no shell involved with OPEN-PIPE*, only
> with OPEN-PIPE.  Sorry.)

No problem. I'm sorry I'm the one who asked a ton of questions.

>> I will just write what you have told me, but may I continue to
>> modify
>> the patch?
>
> Of course!  Curious how, though.

I have also received a response from Maxime and plan to include the
following information.

(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."
  (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"))
  (let ((pager-command-line (find-available-pager)))
...

However, I can't submit the v2 patch yet because I don't know how to
implement the integration test.

Thanks,
-- 
Taiju




Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Wed, 08 Jun 2022 15:10:02 GMT) Full text and rfc822 format available.

Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Wed, 08 Jun 2022 15:18:01 GMT) Full text and rfc822 format available.

Message #41 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Taiju HIGASHI <higashi <at> taiju.info>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 55845 <at> debbugs.gnu.org
Subject: Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when
 less is not installed.
Date: Thu, 09 Jun 2022 00:17:19 +0900
Maxime Devos <maximedevos <at> 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




Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Wed, 08 Jun 2022 16:48:01 GMT) Full text and rfc822 format available.

Message #44 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Taiju HIGASHI <higashi <at> taiju.info>
Cc: 55845 <at> debbugs.gnu.org
Subject: Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when
 less is not installed.
Date: Wed, 08 Jun 2022 18:46:45 +0200
[Message part 1 (text/plain, inline)]
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.
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Thu, 09 Jun 2022 09:53:02 GMT) Full text and rfc822 format available.

Message #47 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Taiju HIGASHI <higashi <at> taiju.info>
To: Maxime Devos <maximedevos <at> telenet.be>, me <at> tobias.gr
Cc: 55845 <at> debbugs.gnu.org
Subject: Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when
 less is not installed.
Date: Thu, 09 Jun 2022 18:52:11 +0900
[Message part 1 (text/plain, inline)]
Hi,

I have created a v2 patch and have attached it to this email and also
added a unit test for the find-available-pager.
[v2-0001-ui-Improve-pager-selection-logic-when-less-is-not.patch (text/x-patch, attachment)]
[Message part 3 (text/plain, inline)]
Please check it out.

Regards,
-- 
Taiju

Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Thu, 09 Jun 2022 10:24:02 GMT) Full text and rfc822 format available.

Message #50 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Taiju HIGASHI <higashi <at> taiju.info>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 55845 <at> debbugs.gnu.org, me <at> tobias.gr
Subject: Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when
 less is not installed.
Date: Thu, 09 Jun 2022 19:23:20 +0900
Hi Maxime,

I tried to mock open-pipe* and isatty?* using the mock macro and also
add a test to inspect the program coming across to open-pipe*, but gave
up because I could not get the return value of the
with-paginated-output-port macro.

I think we are one step closer, but it is not working.
I will share a piece of code in the process of verification just in
case.

(test-equal "with-paginated-output-port"
  "less"
  (call-with-temporary-directory
   (lambda (dir)
     (with-environment-variables
         `(("PATH" ,dir))
       (make-dummy-executable-file dir "less")
       (mock ((ice-9 popen) open-pipe*
              (lambda (mode command . args)
                (current-output-port)))
             (mock ((guix colors) isatty?* (const #t))
                   (with-paginated-output-port paginated
                     "less")))))))

I have debugged that the return value of dynamic-wind is "less", but I
could not successfully use it for assertions.

I also tried to inspect the value of the command argument using
test-equal in the open-pipe* mock replacement function, but it did not
work.

Is there a better way to do this?

Thanks,
-- 
Taiju




Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Thu, 09 Jun 2022 19:44:01 GMT) Full text and rfc822 format available.

Message #53 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Taiju HIGASHI <higashi <at> taiju.info>
Cc: 55845 <at> debbugs.gnu.org, me <at> tobias.gr
Subject: Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when
 less is not installed.
Date: Thu, 09 Jun 2022 21:43:11 +0200
[Message part 1 (text/plain, inline)]
Taiju HIGASHI schreef op do 09-06-2022 om 19:23 [+0900]:
> Hi Maxime,
> 
> I tried to mock open-pipe* and isatty?* using the mock macro and also
> add a test to inspect the program coming across to open-pipe*, but gave
> up because I could not get the return value of the
> with-paginated-output-port macro.

The return value of 'with-paginated-output-port' is just whatever the
last expression put in that macro evaluates to.  Also 'close-pipe'
needs to be mocked, otherwise an error will result.

Try:

(test-assert "with-paginated-output-port: finds less in PATH"
  (call-with-temporary-directory
    (lambda (dir)
      (define used-command #false)
      (with-environment-variables
          `(("PATH" ,dir))
        (make-dummy-executable-file dir "less")
        (mock ((ice-9 popen) open-pipe*
               (lambda (mode command . args)
                 (when used-command ; <--- an extra test
                    (error "open-pipe* should only be called once"))
                 (set! used-command command) ; <--- this captures the passed command
                 (%make-void-port ""))) ; return a dummy port
              (mock ((ice-9 popen) close-pipe (const 'ok))
                 (mock ((guix colors) isatty?* (const #t))
                    (with-paginated-output-port port 'ok)))))
      (and (pk 'used-command used-command dir) ; <-- fails on my computer because a non-absolute path is passed and I haven't applied our patch
           (string=? (in-vicinity dir "less") used-command)))))

Greetings,
Maxime.
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Fri, 10 Jun 2022 00:40:02 GMT) Full text and rfc822 format available.

Message #56 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Taiju HIGASHI <higashi <at> taiju.info>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 55845 <at> debbugs.gnu.org, me <at> tobias.gr
Subject: Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when
 less is not installed.
Date: Fri, 10 Jun 2022 09:39:24 +0900
[Message part 1 (text/plain, inline)]
Maxime Devos <maximedevos <at> telenet.be> writes:

> Taiju HIGASHI schreef op do 09-06-2022 om 19:23 [+0900]:
>> Hi Maxime,
>>
>> I tried to mock open-pipe* and isatty?* using the mock macro and also
>> add a test to inspect the program coming across to open-pipe*, but gave
>> up because I could not get the return value of the
>> with-paginated-output-port macro.
>
> The return value of 'with-paginated-output-port' is just whatever the
> last expression put in that macro evaluates to.  Also 'close-pipe'
> needs to be mocked, otherwise an error will result.
>
> Try:
>
> (test-assert "with-paginated-output-port: finds less in PATH"
>   (call-with-temporary-directory
>     (lambda (dir)
>       (define used-command #false)
>       (with-environment-variables
>           `(("PATH" ,dir))
>         (make-dummy-executable-file dir "less")
>         (mock ((ice-9 popen) open-pipe*
>                (lambda (mode command . args)
>                  (when used-command ; <--- an extra test
>                     (error "open-pipe* should only be called once"))
>                  (set! used-command command) ; <--- this captures the passed command
>                  (%make-void-port ""))) ; return a dummy port
>               (mock ((ice-9 popen) close-pipe (const 'ok))
>                  (mock ((guix colors) isatty?* (const #t))
>                     (with-paginated-output-port port 'ok)))))
>       (and (pk 'used-command used-command dir) ; <-- fails on my computer because a non-absolute path is passed and I haven't applied our patch
>            (string=? (in-vicinity dir "less") used-command)))))

Thank you very much! It worked as expected!

I made a v3 patch.
[v3-0001-ui-Improve-pager-selection-logic-when-less-is-not.patch (text/x-patch, attachment)]
[Message part 3 (text/plain, inline)]
Two tests have been added: one to select pager from the environment
variable and the other to select less from the PATH.

I also made some improvements to the existing tests based on your
answers.

There are many ways to do this. I learned a lot.

Thanks,
-- 
Taiju

Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Fri, 10 Jun 2022 00:57:01 GMT) Full text and rfc822 format available.

Message #59 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Taiju HIGASHI <higashi <at> taiju.info>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 55845 <at> debbugs.gnu.org, me <at> tobias.gr
Subject: Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when
 less is not installed.
Date: Fri, 10 Jun 2022 09:55:52 +0900
This is off-topic.

I feel I have a limited vocabulary available to me in Guile or Scheme
(as well as in English...) , but functions like pk were not included in
Guile's reference or Scheme's reference, so I thought my chances of
knowing them were quite limited. (I didn't know about in-vicinity
either, but now that I know it is in SRFI.)

Are these the kind of things you learn by reading the source?

-- 
Taiju




Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Fri, 10 Jun 2022 07:38:01 GMT) Full text and rfc822 format available.

Message #62 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Taiju HIGASHI <higashi <at> taiju.info>
Cc: 55845 <at> debbugs.gnu.org, me <at> tobias.gr
Subject: Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when
 less is not installed.
Date: Fri, 10 Jun 2022 09:37:44 +0200
[Message part 1 (text/plain, inline)]
Taiju HIGASHI schreef op vr 10-06-2022 om 09:55 [+0900]:
> I feel I have a limited vocabulary available to me in Guile or Scheme
> (as well as in English...) , but functions like pk were not included in
> Guile's reference or Scheme's reference, so I thought my chances of
> knowing them were quite limited. (I didn't know about in-vicinity
> either, but now that I know it is in SRFI.)

Didn't know it was part of a SRFI.

> Are these the kind of things you learn by reading the source?

Yes, some things aren't documented.  Though if interested, feel free to
doucment them.

Greetings,
Maxime.
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Fri, 10 Jun 2022 07:48:01 GMT) Full text and rfc822 format available.

Message #65 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Taiju HIGASHI <higashi <at> taiju.info>
Cc: 55845 <at> debbugs.gnu.org, me <at> tobias.gr
Subject: Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when
 less is not installed.
Date: Fri, 10 Jun 2022 09:47:30 +0200
[Message part 1 (text/plain, inline)]
Taiju HIGASHI schreef op vr 10-06-2022 om 09:39 [+0900]:
> +     (with-environment-variables
> +         `(("PATH" ,dir))

Wait, looking at the definition of with-environment-variables, if PAGER
was set when running "make check" (try "GUIX_PAGER=less make check
TESTS=tests/ui.scm"), it will still be set when the test is run
(unverified).  Maybe it should be unset?  Proposal:

Change

                  (match-lambda
                    ((variable value)
                     (setenv variable value)

to

                  (match-lambda
                    ((variable #false)
                     (unsetenv variable))
                    ((variable value)
                     (setenv variable value)


and change the with-environment-variables to

   (with-environment-variables
      `(("PATH ,dir)
        ("PAGER" #false)
        ("GUIX_PAGER" #false))
      [...]).

and likewise for the other tests?

(string=? ((@@ (guix ui) find-available-pager)) "guix-pager")))))

Nitpick: find-available-pager is not an exported procedure, so due to
optimisations, it can dissappear (be inlined into call-with-paginated-
output-port).  So to be 100% robust, it needs to be exported, or a line

  (set! find-avaible-pager find-available-pager) ; used in tests/ui.scm

needs to be added in guix/ui.scm, or the tests needs to be adjusted
to always use with-paginated-output-port instead of
find-available-pager.

Otherwise, tests LGTM.

Greetings,
Maxime.
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Fri, 10 Jun 2022 08:41:02 GMT) Full text and rfc822 format available.

Message #68 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Taiju HIGASHI <higashi <at> taiju.info>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 55845 <at> debbugs.gnu.org, me <at> tobias.gr
Subject: Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when
 less is not installed.
Date: Fri, 10 Jun 2022 17:40:24 +0900
Maxime Devos <maximedevos <at> telenet.be> writes:

> Taiju HIGASHI schreef op vr 10-06-2022 om 09:39 [+0900]:
>> +     (with-environment-variables
>> +         `(("PATH" ,dir))
>
> Wait, looking at the definition of with-environment-variables, if PAGER
> was set when running "make check" (try "GUIX_PAGER=less make check
> TESTS=tests/ui.scm"), it will still be set when the test is run
> (unverified).  Maybe it should be unset?  Proposal:
>
> Change
>
>                   (match-lambda
>                     ((variable value)
>                      (setenv variable value)
>
> to
>
>                   (match-lambda
>                     ((variable #false)
>                      (unsetenv variable))
>                     ((variable value)
>                      (setenv variable value)
>
>
> and change the with-environment-variables to
>
>    (with-environment-variables
>       `(("PATH ,dir)
>         ("PAGER" #false)
>         ("GUIX_PAGER" #false))
>       [...]).
>
> and likewise for the other tests?

Sorry, I easily used with-environment-variable*s* because the interface
looked convenient, but perhaps I should have used
with-environment-variable defined in guix/tests.scm.
However, using this one does not seem to solve the problem. Should I
modify with-environment-variable*s*?

> (string=? ((@@ (guix ui) find-available-pager)) "guix-pager")))))
>
> Nitpick: find-available-pager is not an exported procedure, so due to
> optimisations, it can dissappear (be inlined into call-with-paginated-
> output-port).  So to be 100% robust, it needs to be exported, or a line
>
>   (set! find-avaible-pager find-available-pager) ; used in tests/ui.scm
>
> needs to be added in guix/ui.scm, or the tests needs to be adjusted
> to always use with-paginated-output-port instead of
> find-available-pager.


Thank you. I will modify it in one of the ways you suggested.

I did not understand the optimization behavior. Thank you very much.

Thanks,
-- 
Taiju




Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Fri, 10 Jun 2022 08:53:01 GMT) Full text and rfc822 format available.

Message #71 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Taiju HIGASHI <higashi <at> taiju.info>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 55845 <at> debbugs.gnu.org, me <at> tobias.gr
Subject: Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when
 less is not installed.
Date: Fri, 10 Jun 2022 17:52:40 +0900
> Taiju HIGASHI schreef op vr 10-06-2022 om 09:55 [+0900]:
>> I feel I have a limited vocabulary available to me in Guile or Scheme
>> (as well as in English...) , but functions like pk were not included in
>> Guile's reference or Scheme's reference, so I thought my chances of
>> knowing them were quite limited. (I didn't know about in-vicinity
>> either, but now that I know it is in SRFI.)
>
> Didn't know it was part of a SRFI.

Yes, but in Guile it behaves in a way that makes it useful for
constructing path strings, but when I checked the SRFI specification[0],
the behavior seems to be different from that of the SRFI specification.
(in-vicinity = string-append)

I wonder if this is a subtle specification, since Gauche, which
implements many SRFIs, did not have it either...

>> Are these the kind of things you learn by reading the source?
>
> Yes, some things aren't documented.  Though if interested, feel free to
> doucment them.

Thank you!
This is not a complaint about the documentation. I just wanted to know
how people as proficient in Guile as you guys learned Guile.
I am glad to know that I can learn by getting suggestions for better
code implementation through code reviews, etc.

But, I thought it would be a good idea to include functions like pk in
the documentation, since the efficiency of development depends on
knowing them.

[0]: https://srfi.schemers.org/srfi-59/srfi-59.html

Thanks,
-- 
Taiju




Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Fri, 10 Jun 2022 15:09:02 GMT) Full text and rfc822 format available.

Message #74 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Taiju HIGASHI <higashi <at> taiju.info>
Cc: 55845 <at> debbugs.gnu.org, me <at> tobias.gr
Subject: Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when
 less is not installed.
Date: Fri, 10 Jun 2022 17:08:57 +0200
[Message part 1 (text/plain, inline)]
Taiju HIGASHI schreef op vr 10-06-2022 om 17:40 [+0900]:
> Sorry, I easily used with-environment-variable*s* because the interface
> looked convenient, but perhaps I should have used
> with-environment-variable defined in guix/tests.scm.
> However, using this one does not seem to solve the problem. Should I
> modify with-environment-variable*s*?

I didn't know about 'with-environment-variable' (it already does the
unsetenv!). Just use whatever works (a nested with-environment-variable
or a modified with-environment-variables), though FWIW I would expect
using the modified with-environment-variables to result in more compact
code.

Greetings,
Maxime
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Sat, 11 Jun 2022 11:27:02 GMT) Full text and rfc822 format available.

Message #77 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Taiju HIGASHI <higashi <at> taiju.info>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 55845 <at> debbugs.gnu.org, me <at> tobias.gr
Subject: Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when
 less is not installed.
Date: Sat, 11 Jun 2022 20:26:12 +0900
[Message part 1 (text/plain, inline)]
Hi Maxime,

>> Sorry, I easily used with-environment-variable*s* because the interface
>> looked convenient, but perhaps I should have used
>> with-environment-variable defined in guix/tests.scm.
>> However, using this one does not seem to solve the problem. Should I
>> modify with-environment-variable*s*?
>
> I didn't know about 'with-environment-variable' (it already does the
> unsetenv!). Just use whatever works (a nested with-environment-variable
> or a modified with-environment-variables), though FWIW I would expect
> using the modified with-environment-variables to result in more compact
> code.

I have attached the v4 patch.
[v4-0001-ui-Improve-pager-selection-logic-when-less-is-not.patch (text/x-patch, attachment)]
[Message part 3 (text/plain, inline)]
I decided to fix with-environment-variables.

Also, the problem of find-available-pager tests being tests for
unexported functions has been changed to tests that use the exported
with-paginated-output-port to verify that the command is passed to
open-pipe* as expected.

Regards,
-- 
Taiju

Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Wed, 15 Jun 2022 00:00:02 GMT) Full text and rfc822 format available.

Message #80 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Taiju HIGASHI <higashi <at> taiju.info>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 55845 <at> debbugs.gnu.org, me <at> tobias.gr
Subject: Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when
 less is not installed.
Date: Wed, 15 Jun 2022 08:58:57 +0900
Hi Maxime,

Please let me know if you have any problems with the v4 patch I sent you
a few days ago :)

Thanks,
-- 
Taiju




Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Wed, 15 Jun 2022 08:03:01 GMT) Full text and rfc822 format available.

Message #83 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Taiju HIGASHI <higashi <at> taiju.info>
Cc: 55845 <at> debbugs.gnu.org, me <at> tobias.gr
Subject: Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when
 less is not installed.
Date: Wed, 15 Jun 2022 10:02:26 +0200
[Message part 1 (text/plain, inline)]
Taiju HIGASHI schreef op wo 15-06-2022 om 08:58 [+0900]:
>  Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed. (Van: Taiju HIGASHI <higashi <at> taiju.info>)Van:Taiju HIGASHI <higashi <at> taiju.info>Aan:Maxime Devos <maximedevos <at> telenet.be>Cc:me <at> tobias.gr, 55845 <at> debbugs.gnu.orgOnderwerp:Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.Datum:Wed, 15 Jun 2022 08:58:57 +0900 (15-06-22 01:58:57)

The patches look fine from here (only reading them, not actually
running the tests myself).

Greetings,
MMaxime.
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Thu, 16 Jun 2022 21:44:02 GMT) Full text and rfc822 format available.

Message #86 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Taiju HIGASHI <higashi <at> taiju.info>
Cc: 55845 <at> debbugs.gnu.org, me <at> tobias.gr, Maxime Devos <maximedevos <at> telenet.be>
Subject: Re: bug#55845: [PATCH 0/1] Improve pager selection logic when less
 is not installed
Date: Thu, 16 Jun 2022 23:43:46 +0200
[Message part 1 (text/plain, inline)]
Hi,

Taiju HIGASHI <higashi <at> taiju.info> skribis:

>>From bf557600c549e22a06ccfb288b89b1a0736b0500 Mon Sep 17 00:00:00 2001
> From: Taiju HIGASHI <higashi <at> taiju.info>
> Date: Wed, 8 Jun 2022 18:50:28 +0900
> Subject: [PATCH v4] ui: Improve pager selection logic when less is not
>  installed.
>
> * guix/ui.scm (find-available-pager): New procedure. Return a available pager.
>   (call-with-paginated-output-port): Change to use find-available-pager to
>   select pager.
> * guix/utils.scm (call-with-environment-variables): Allow clearing of
> specified environment variables.
> * tests/ui.scm: Add tests for find-available-pager.

Applied with the cosmetic changes below, mostly aiming to visually
simplify the code and make it consistent with the rest.

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!

Ludo’.

[Message part 2 (text/x-patch, inline)]
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")

bug closed, send any further explanations to 55845 <at> debbugs.gnu.org and Taiju HIGASHI <higashi <at> taiju.info> Request was from Ludovic Courtès <ludo <at> gnu.org> to control <at> debbugs.gnu.org. (Thu, 16 Jun 2022 21:45:01 GMT) Full text and rfc822 format available.

Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Fri, 17 Jun 2022 00:39:01 GMT) Full text and rfc822 format available.

Message #91 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Taiju HIGASHI <higashi <at> taiju.info>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 55845 <at> debbugs.gnu.org, me <at> tobias.gr, Maxime Devos <maximedevos <at> telenet.be>
Subject: Re: bug#55845: [PATCH 0/1] Improve pager selection logic when less
 is not installed
Date: Fri, 17 Jun 2022 09:38:16 +0900
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




Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Fri, 17 Jun 2022 12:40:02 GMT) Full text and rfc822 format available.

Message #94 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Taiju HIGASHI <higashi <at> taiju.info>
Cc: 55845 <at> debbugs.gnu.org, me <at> tobias.gr, Maxime Devos <maximedevos <at> telenet.be>
Subject: Re: bug#55845: [PATCH 0/1] Improve pager selection logic when less
 is not installed
Date: Fri, 17 Jun 2022 14:39:37 +0200
Hello,

Taiju HIGASHI <higashi <at> 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’.




Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Fri, 17 Jun 2022 13:37:02 GMT) Full text and rfc822 format available.

Message #97 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Taiju HIGASHI <higashi <at> taiju.info>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 55845 <at> debbugs.gnu.org, me <at> tobias.gr, Maxime Devos <maximedevos <at> telenet.be>
Subject: Re: bug#55845: [PATCH 0/1] Improve pager selection logic when less
 is not installed
Date: Fri, 17 Jun 2022 22:36:07 +0900
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,
-- 
Taiju




Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Fri, 17 Jun 2022 15:13:02 GMT) Full text and rfc822 format available.

Message #100 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Taiju HIGASHI <higashi <at> taiju.info>
Cc: 55845 <at> debbugs.gnu.org, me <at> tobias.gr, Maxime Devos <maximedevos <at> telenet.be>
Subject: Re: bug#55845: [PATCH 0/1] Improve pager selection logic when less
 is not installed
Date: Fri, 17 Jun 2022 17:12:26 +0200
Hi!

Taiju HIGASHI <higashi <at> 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’.




Information forwarded to guix-patches <at> gnu.org:
bug#55845; Package guix-patches. (Sat, 18 Jun 2022 14:12:02 GMT) Full text and rfc822 format available.

Message #103 received at 55845 <at> debbugs.gnu.org (full text, mbox):

From: Taiju HIGASHI <higashi <at> taiju.info>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 55845 <at> debbugs.gnu.org, me <at> tobias.gr, Maxime Devos <maximedevos <at> telenet.be>
Subject: Re: bug#55845: [PATCH 0/1] Improve pager selection logic when less
 is not installed
Date: Sat, 18 Jun 2022 23:11:48 +0900
>> 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.)
-- 
Taiju




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Sun, 17 Jul 2022 11:24:04 GMT) Full text and rfc822 format available.

This bug report was last modified 1 year and 255 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.