|Subject:||Re: [PATCH] substitute: Improve readability of substitute-related output.|
|Date:||Tue, 15 Sep 2015 22:00:06 -0700|
Steve Sprang <address@hidden> skribis:
> I added newlines to split up output and visually separate multiple
> substitutions. Since the download size is unknown, we can't use a progress
> bar, but I made the output more consistent with the progress bar behavior.
> I also shortened the hash after it's been repeated in full twice (once for
> the store and once for the download URL).
> Patched behavior:
> Found valid signature for
> From http://hydra.gnu.org/nar/sin6c3n1440f8kza0k4hdms38fbb4dv0-boost-1.58.0
> Downloading sin6c3…boost-1.58.0 (110.6MiB installed)...
> sin6c3…boost-1.58.0 911KiB/s 00:00:11 | 9.4MiB
Looks good to me.
Regarding elapsed time, I wonder if we should get rid of hours–i.e.,
print “00:11” in this case. Most of the time downloads last less than
an hour, and when it lasts more, it’s OK to either print 100:11 or
> From 95936bf25394d2985f9331cb8fa08d5b30cb64a5 Mon Sep 17 00:00:00 2001
> From: Steve Sprang <address@hidden>
> Date: Mon, 14 Sep 2015 22:31:11 -0700
> Subject: [PATCH] substitute: Improve readability of substitute-related output.
> * guix/build/download.scm (flexible-space, truncated-url): New procedures.
> (progress-proc): Generate a better indeterminate progress string.
> (nearest-exact-integer, seconds->string, byte-count->string): Move to...
> * guix/utils.scm: ...here.
> * guix/store.scm (truncated-store-path): New procedure.
> * guix/scripts/substitute.scm (assert-valid-narinfo): Add newlines to output.
> (process-substitution): Use byte-count->string and truncated-store-path.
In general, it is best to make a separate patch that simply moves
procedures from one file to another.
However, most of the time, (guix build …) modules must not use (guix …)
modules (info "(guix) G-Expressions"). This is the case here.
So what I would suggest is to export those helper procedures from (guix
build download) itself, as is done for ‘progress-proc’ et al.
> +(define* (flexible-space left right #:optional (columns 80))
> + "Return a string of spaces which can be used to separate LEFT and RIGHT so
> +that RIGHT is justified to a width of COLUMNS."
What about this slightly more idiomatic procedure instead:
(define (string-pad-middle left right len)
;; + docstring
(string-pad right (max 0 (- len (string-length left))))))
> +(define (truncated-url url)
> + "Return a friendlier version of URL for display."
> + (let ((store-path (string-append (%store-prefix) "/" (basename url))))
> + ;; take advantage of the implementation for store paths
> + (truncated-store-path store-path)))
What about calling it ‘store-url-abbreviation’, to convey that it’s
similar in spirit to ‘uri-abbreviation’, but specialized?
> (define* (progress-proc file size #:optional (log-port (current-output-port)))
> "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
> @@ -130,24 +115,26 @@ bytes long. The returned procedure is suitable for use as an argument to
> (seconds->string elapsed)
> (progress-bar %) %))
> ;; TODO: Make this adapt to the actual terminal width.
> - (cols 80)
> - (num-spaces (max 1 (- cols (+ (string-length left)
> - (string-length right)))))
> - (gap (make-string num-spaces #\space)))
> + (gap (flexible-space left right)))
> (format log-port "~a~a~a" left gap right)
This would become:
(display (string-pad-middle left right cols) log-port)
Could you send an updated patch?
Description: Text Data
|[Prev in Thread]||Current Thread||[Next in Thread]|