[PATCH] download: Fix some minor progress-logging regressions.

[PATCH] download: Fix some minor progress-logging regressions.

[Steve Sprang]

[-- Attachment #1.1: Type: text/plain, Size: 50 bytes --]

Just some small tweaks to fix up logging.

-Steve

[-- Attachment #1.2: Type: text/html, Size: 92 bytes --]

[-- Attachment #2: progress-fix.patch --]
[-- Type: text/x-patch, Size: 2683 bytes --]

From 11440f61cebd49d31d165f6433ec6e4b1afe728f Mon Sep 17 00:00:00 2001
From: Steve Sprang <scs@stevesprang.com>
Date: Thu, 17 Sep 2015 04:22:01 -0700
Subject: [PATCH] download: Fix some minor progress-logging regressions.

* guix/build/download.scm
  (string-pad-middle): Allow resulting padded string to overflow.
  (store-url-abbreviation): Remove unnecessary procedure.
  (progress-proc): Default abbreviation should be basename.
  (url-fetch): Insert some newlines for readability.
---
 guix/build/download.scm | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/guix/build/download.scm b/guix/build/download.scm
index 9b72e8f..e6de4d2 100644
--- a/guix/build/download.scm
+++ b/guix/build/download.scm
@@ -100,15 +100,13 @@ width of the bar is BAR-WIDTH."
 
 (define (string-pad-middle left right len)
   "Combine LEFT and RIGHT with enough padding in the middle so that the
-resulting string has length at least LEN.  This right justifies RIGHT."
-  (string-append left
-                 (string-pad right (max 0 (- len (string-length left))))))
-
-(define (store-url-abbreviation url)
-  "Return a friendlier version of URL for display."
-  (let ((store-path (string-append (%store-directory) "/" (basename url))))
-    ;; Take advantage of the implementation for store paths.
-    (store-path-abbreviation store-path)))
+resulting string has length at least LEN (it may overflow).  This right
+justifies RIGHT."
+  (let* ((total-used (+ (string-length left)
+                        (string-length right)))
+         (num-spaces (max 1 (- len total-used)))
+         (padding    (make-string num-spaces #\space)))
+    (string-append left padding right)))
 
 (define* (store-path-abbreviation store-path #:optional (prefix-length 6))
   "Return an abbreviation of STORE-PATH for display, showing PREFIX-LENGTH
@@ -120,7 +118,7 @@ characters of the hash."
 
 (define* (progress-proc file size
                         #:optional (log-port (current-output-port))
-                        #:key (abbreviation identity))
+                        #:key (abbreviation basename))
   "Return a procedure to show the progress of FILE's download, which is SIZE
 bytes long.  The returned procedure is suitable for use as an argument to
 `dump-port'.  The progress report is written to LOG-PORT, with ABBREVIATION
@@ -518,7 +516,7 @@ on success."
                   (_       (list (string->uri url))))))
 
   (define (fetch uri file)
-    (format #t "starting download of `~a' from `~a'...~%"
+    (format #t "~%Starting download of ~a~%From ~a...~%"
             file (uri->string uri))
     (case (uri-scheme uri)
       ((http https)
-- 
2.5.0

Re: [PATCH] download: Fix some minor progress-logging regressions.

[Mathieu Lirzin]

Steve Sprang <steve.sprang@gmail.com> writes:

> From 11440f61cebd49d31d165f6433ec6e4b1afe728f Mon Sep 17 00:00:00 2001
> From: Steve Sprang <scs@stevesprang.com>
> Date: Thu, 17 Sep 2015 04:22:01 -0700
> Subject: [PATCH] download: Fix some minor progress-logging regressions.
>
> * guix/build/download.scm
>   (string-pad-middle): Allow resulting padded string to overflow.
>   (store-url-abbreviation): Remove unnecessary procedure.
>   (progress-proc): Default abbreviation should be basename.

What about something like "(progress-proc): Use BASENAME as default for
parameter 'abbreviation'." ?

>   (url-fetch): Insert some newlines for readability.

I would prefer "Display" instead of "Insert" to not confuse with
insertion of newlines in the source code (even if such change won't be
notified in the commit log) :)

> ---
>  guix/build/download.scm | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/guix/build/download.scm b/guix/build/download.scm
> index 9b72e8f..e6de4d2 100644
> --- a/guix/build/download.scm
> +++ b/guix/build/download.scm
> @@ -100,15 +100,13 @@ width of the bar is BAR-WIDTH."
>  
>  (define (string-pad-middle left right len)
>    "Combine LEFT and RIGHT with enough padding in the middle so that the
> -resulting string has length at least LEN.  This right justifies RIGHT."
                                              ^^^
Maybe it's only because of my poor english but I don't understand the
meaning of the last sentence.

Otherwise this looks good to me.

--
Mathieu Lirzin

Re: [PATCH] download: Fix some minor progress-logging regressions.

[Steve Sprang]

On Thu, Sep 17, 2015 at 11:18 AM, Mathieu Lirzin <mthl@openmailbox.org> wrote:
> Steve Sprang <steve.sprang@gmail.com> writes:
>
>> From 11440f61cebd49d31d165f6433ec6e4b1afe728f Mon Sep 17 00:00:00 2001
>> From: Steve Sprang <scs@stevesprang.com>
>> Date: Thu, 17 Sep 2015 04:22:01 -0700
>> Subject: [PATCH] download: Fix some minor progress-logging regressions.
>>
>> * guix/build/download.scm
>>   (string-pad-middle): Allow resulting padded string to overflow.
>>   (store-url-abbreviation): Remove unnecessary procedure.
>>   (progress-proc): Default abbreviation should be basename.
>
> What about something like "(progress-proc): Use BASENAME as default for
> parameter 'abbreviation'." ?

Ok, sounds good.

>>   (url-fetch): Insert some newlines for readability.
>
> I would prefer "Display" instead of "Insert" to not confuse with
> insertion of newlines in the source code (even if such change won't be
> notified in the commit log) :)

Ok.

>> ---
>>  guix/build/download.scm | 20 +++++++++-----------
>>  1 file changed, 9 insertions(+), 11 deletions(-)
>>
>> diff --git a/guix/build/download.scm b/guix/build/download.scm
>> index 9b72e8f..e6de4d2 100644
>> --- a/guix/build/download.scm
>> +++ b/guix/build/download.scm
>> @@ -100,15 +100,13 @@ width of the bar is BAR-WIDTH."
>>
>>  (define (string-pad-middle left right len)
>>    "Combine LEFT and RIGHT with enough padding in the middle so that the
>> -resulting string has length at least LEN.  This right justifies RIGHT."
>                                               ^^^
> Maybe it's only because of my poor english but I don't understand the
> meaning of the last sentence.

Yes, the phrasing is awkward. Basically the effect is that the RIGHT
string will be flush with the right edge when the string is LEN width
(i.e. it is right-justified as in typography). I will try to phrase it
better.

I have to head out for the moment, but I will send a tweaked patch later today.

-Steve

> Otherwise this looks good to me.
>
> --
> Mathieu Lirzin

Re: [PATCH] download: Fix some minor progress-logging regressions.

[Steve Sprang]

[-- Attachment #1: Type: text/plain, Size: 2188 bytes --]

Here's attempt #2.

-Steve

On Thu, Sep 17, 2015 at 12:44 PM, Steve Sprang <steve.sprang@gmail.com> wrote:
> On Thu, Sep 17, 2015 at 11:18 AM, Mathieu Lirzin <mthl@openmailbox.org> wrote:
>> Steve Sprang <steve.sprang@gmail.com> writes:
>>
>>> From 11440f61cebd49d31d165f6433ec6e4b1afe728f Mon Sep 17 00:00:00 2001
>>> From: Steve Sprang <scs@stevesprang.com>
>>> Date: Thu, 17 Sep 2015 04:22:01 -0700
>>> Subject: [PATCH] download: Fix some minor progress-logging regressions.
>>>
>>> * guix/build/download.scm
>>>   (string-pad-middle): Allow resulting padded string to overflow.
>>>   (store-url-abbreviation): Remove unnecessary procedure.
>>>   (progress-proc): Default abbreviation should be basename.
>>
>> What about something like "(progress-proc): Use BASENAME as default for
>> parameter 'abbreviation'." ?
>
> Ok, sounds good.
>
>>>   (url-fetch): Insert some newlines for readability.
>>
>> I would prefer "Display" instead of "Insert" to not confuse with
>> insertion of newlines in the source code (even if such change won't be
>> notified in the commit log) :)
>
> Ok.
>
>>> ---
>>>  guix/build/download.scm | 20 +++++++++-----------
>>>  1 file changed, 9 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/guix/build/download.scm b/guix/build/download.scm
>>> index 9b72e8f..e6de4d2 100644
>>> --- a/guix/build/download.scm
>>> +++ b/guix/build/download.scm
>>> @@ -100,15 +100,13 @@ width of the bar is BAR-WIDTH."
>>>
>>>  (define (string-pad-middle left right len)
>>>    "Combine LEFT and RIGHT with enough padding in the middle so that the
>>> -resulting string has length at least LEN.  This right justifies RIGHT."
>>                                               ^^^
>> Maybe it's only because of my poor english but I don't understand the
>> meaning of the last sentence.
>
> Yes, the phrasing is awkward. Basically the effect is that the RIGHT
> string will be flush with the right edge when the string is LEN width
> (i.e. it is right-justified as in typography). I will try to phrase it
> better.
>
> I have to head out for the moment, but I will send a tweaked patch later today.
>
> -Steve
>
>> Otherwise this looks good to me.
>>
>> --
>> Mathieu Lirzin

[-- Attachment #2: progress-fix-2.patch --]
[-- Type: text/x-patch, Size: 2764 bytes --]

From 65fe041c63a58ae5c72ef33f05349ca04afa470b Mon Sep 17 00:00:00 2001
From: Steve Sprang <scs@stevesprang.com>
Date: Thu, 17 Sep 2015 04:22:01 -0700
Subject: [PATCH] download: Fix some minor progress-logging regressions.

* guix/build/download.scm
  (string-pad-middle): Allow resulting padded string to overflow.
  (store-url-abbreviation): Remove unnecessary procedure.
  (progress-proc): Use BASENAME as default for parameter 'abbreviation'.
  (url-fetch): Display extra newlines for readability.
---
 guix/build/download.scm | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/guix/build/download.scm b/guix/build/download.scm
index d362fc1..be4a581 100644
--- a/guix/build/download.scm
+++ b/guix/build/download.scm
@@ -101,15 +101,14 @@ width of the bar is BAR-WIDTH."
 
 (define (string-pad-middle left right len)
   "Combine LEFT and RIGHT with enough padding in the middle so that the
-resulting string has length at least LEN.  This right justifies RIGHT."
-  (string-append left
-                 (string-pad right (max 0 (- len (string-length left))))))
-
-(define (store-url-abbreviation url)
-  "Return a friendlier version of URL for display."
-  (let ((store-path (string-append (%store-directory) "/" (basename url))))
-    ;; Take advantage of the implementation for store paths.
-    (store-path-abbreviation store-path)))
+resulting string has length at least LEN (it may overflow).  If the string
+does not overflow, the last char in RIGHT will be flush with the LEN
+column."
+  (let* ((total-used (+ (string-length left)
+                        (string-length right)))
+         (num-spaces (max 1 (- len total-used)))
+         (padding    (make-string num-spaces #\space)))
+    (string-append left padding right)))
 
 (define* (store-path-abbreviation store-path #:optional (prefix-length 6))
   "Return an abbreviation of STORE-PATH for display, showing PREFIX-LENGTH
@@ -121,7 +120,7 @@ characters of the hash."
 
 (define* (progress-proc file size
                         #:optional (log-port (current-output-port))
-                        #:key (abbreviation identity))
+                        #:key (abbreviation basename))
   "Return a procedure to show the progress of FILE's download, which is SIZE
 bytes long.  The returned procedure is suitable for use as an argument to
 `dump-port'.  The progress report is written to LOG-PORT, with ABBREVIATION
@@ -519,7 +518,7 @@ on success."
                   (_       (list (string->uri url))))))
 
   (define (fetch uri file)
-    (format #t "starting download of `~a' from `~a'...~%"
+    (format #t "~%Starting download of ~a~%From ~a...~%"
             file (uri->string uri))
     (case (uri-scheme uri)
       ((http https)
-- 
2.5.0

Re: [PATCH] download: Fix some minor progress-logging regressions.

[Mathieu Lirzin]

Steve Sprang <steve.sprang@gmail.com> writes:

> Here's attempt #2.
[...]
> @@ -101,15 +101,14 @@ width of the bar is BAR-WIDTH."
>  
>  (define (string-pad-middle left right len)
>    "Combine LEFT and RIGHT with enough padding in the middle so that the
> -resulting string has length at least LEN.  This right justifies RIGHT."
> -  (string-append left
> -                 (string-pad right (max 0 (- len (string-length left))))))
> -
> -(define (store-url-abbreviation url)
> -  "Return a friendlier version of URL for display."
> -  (let ((store-path (string-append (%store-directory) "/" (basename url))))
> -    ;; Take advantage of the implementation for store paths.
> -    (store-path-abbreviation store-path)))
> +resulting string has length at least LEN (it may overflow).  If the string
> +does not overflow, the last char in RIGHT will be flush with the LEN
> +column."
> +  (let* ((total-used (+ (string-length left)
> +                        (string-length right)))
> +         (num-spaces (max 1 (- len total-used)))
> +         (padding    (make-string num-spaces #\space)))
> +    (string-append left padding right)))

The phrasing is fine for me.  Thanks again.

--
Mathieu Lirzin

Re: [PATCH] download: Fix some minor progress-logging regressions.

[Steve Sprang]

This patch is still pending. I don't want it to fall through the cracks. :-)

Thanks!
-Steve

On Thu, Sep 17, 2015 at 2:44 PM, Mathieu Lirzin <mthl@openmailbox.org> wrote:
> Steve Sprang <steve.sprang@gmail.com> writes:
>
>> Here's attempt #2.
> [...]
>> @@ -101,15 +101,14 @@ width of the bar is BAR-WIDTH."
>>
>>  (define (string-pad-middle left right len)
>>    "Combine LEFT and RIGHT with enough padding in the middle so that the
>> -resulting string has length at least LEN.  This right justifies RIGHT."
>> -  (string-append left
>> -                 (string-pad right (max 0 (- len (string-length left))))))
>> -
>> -(define (store-url-abbreviation url)
>> -  "Return a friendlier version of URL for display."
>> -  (let ((store-path (string-append (%store-directory) "/" (basename url))))
>> -    ;; Take advantage of the implementation for store paths.
>> -    (store-path-abbreviation store-path)))
>> +resulting string has length at least LEN (it may overflow).  If the string
>> +does not overflow, the last char in RIGHT will be flush with the LEN
>> +column."
>> +  (let* ((total-used (+ (string-length left)
>> +                        (string-length right)))
>> +         (num-spaces (max 1 (- len total-used)))
>> +         (padding    (make-string num-spaces #\space)))
>> +    (string-append left padding right)))
>
> The phrasing is fine for me.  Thanks again.
>
> --
> Mathieu Lirzin

Re: [PATCH] download: Fix some minor progress-logging regressions.

[Ludovic Courtès]

Steve Sprang <steve.sprang@gmail.com> skribis:

> This patch is still pending. I don't want it to fall through the cracks. :-)

Thanks for pinging, it had somehow disappeared from my radar.

Ludo’.

Re: [PATCH] download: Fix some minor progress-logging regressions.

[Mark H Weaver]

Steve Sprang <steve.sprang@gmail.com> writes:
> Here's attempt #2.

[...]

> From 65fe041c63a58ae5c72ef33f05349ca04afa470b Mon Sep 17 00:00:00 2001
> From: Steve Sprang <scs@stevesprang.com>
> Date: Thu, 17 Sep 2015 04:22:01 -0700
> Subject: [PATCH] download: Fix some minor progress-logging regressions.

Pushed, thanks!

     Mark

Re: [PATCH] download: Fix some minor progress-logging regressions.

[Mathieu Lirzin]

Steve Sprang <steve.sprang@gmail.com> writes:

> On Thu, Sep 17, 2015 at 11:18 AM, Mathieu Lirzin <mthl@openmailbox.org> wrote:
>> Steve Sprang <steve.sprang@gmail.com> writes:
>>>  (define (string-pad-middle left right len)
>>>    "Combine LEFT and RIGHT with enough padding in the middle so that the
>>> -resulting string has length at least LEN.  This right justifies RIGHT."
>>                                               ^^^
>> Maybe it's only because of my poor english but I don't understand the
>> meaning of the last sentence.
>
> Yes, the phrasing is awkward. Basically the effect is that the RIGHT
> string will be flush with the right edge when the string is LEN width
> (i.e. it is right-justified as in typography). I will try to phrase it
> better.

Ahh! This makes sense finally.  Thanks for the explanation!

--
Mathieu Lirzin