Re: [ELPA] New package: progress-bar

[Philip Kaludercic]

Mariano Montone <marianomontone@gmail.com> writes:

> Hello,

Hi,

> I would like to propose adding this package to ELPA:
> https://github.com/mmontone/emacs-progress-bar/

Here are a few comments on the code:

diff --git a/progress-bar.el b/progress-bar.el
index ab49ca9..f385f5a 100644
--- a/progress-bar.el
+++ b/progress-bar.el
@@ -24,15 +24,17 @@
 
 ;; A progress bar in the echo area.
 ;;
-;; This package contains the basic implementation.  For integration of 
progress-bar
-;; into common Emacs commands and behaviors, install progress-bar-integrations 
package.
+;; This package contains the basic implementation.  For integration of
+;; progress-bar into common Emacs commands and behaviors, install
+;; progress-bar-integrations package.
 ;; 
-;; Usage:
+;;;; Usage:
 ;;
-;; The preferred method for using a progress-bar is via the utility functions:
-;; `dolist-with-progress-bar', `dotimes-with-progress-bar' and 
`mapc-with-progress-bar'.
+;; The preferred method for using a progress-bar is via the utility
+;; functions: `dolist-with-progress-bar', `dotimes-with-progress-bar'
+;; and `mapc-with-progress-bar'.
 ;;
-;; Example:
+;;;; Example:
 ;;
 ;; (dolist-with-progress-bar (x (cl-loop for i from 1 to 10 collect i)
 ;;                              :status-message (list "Started ..."
@@ -40,11 +42,15 @@
 ;;                                                      (format "Processing 
%s..." (progress-bar-data pb)))
 ;;                                                    "Completed!"))
 ;;     (sit-for (seq-random-elt '(0.3 0.4 0.5))))
-;;
-;; TODO:
-;; - Consider putting event notification in call-with-progress-bar instead of 
in the utilities.
-;; - Consider implementing progress-bars with no total-steps specified.
-;; - Consider an option for hiding the progress-bar display after N seconds 
after completion.
+
+;;; TODO:
+
+;; - Consider putting event notification in call-with-progress-bar
+;;   instead of in the utilities.
+;; - Consider implementing progress-bars with no total-steps
+;;   specified.
+;; - Consider an option for hiding the progress-bar display after N
+;;   seconds after completion.
 
 ;;; Code:
 
@@ -57,48 +63,41 @@
 
 ;; Chosen from https://en.wikipedia.org/wiki/Block_Elements and inserted using 
`insert-char' command:
 
-(defcustom progress-bar-char ?▓
+(defcustom progress-bar-char
+  (eval-when-compile (char-from-name "DARK SHADE")) ;as a suggetion
   "Character for drawing progress bars."
-  :type 'character
-  :group 'progress-bar)
+  :type 'character)                    ;not necessary, :group default to the 
last `defgroup'
 
 (defcustom progress-bar-background-char ?░
   "Character for drawing progress bars background."
-  :type 'character
-  :group 'progress-bar)
+  :type 'character)
 
 (defcustom progress-bar-width 35
   "Standard width for progress bars."
-  :type 'integer
-  :group 'progress-bar)
+  :type 'natnum)
 
 (defcustom progress-bar-min-steps 0
   "Minimum number of steps for progress bars to be displayed."
-  :type 'integer
-  :group 'progress-bar)
+  :type 'natnum)
 
 (defcustom progress-bar-display-after-seconds 0
   "Display progress bars only after this number of seconds have passed."
-  :type 'float
-  :group 'progress-bar)
+  :type 'number)                       ;this was a type mismatch, 0 is not a 
flonum
 
-(defcustom progress-bar-format-string " [%d of %d](%d%%%%)"
+(defcustom progress-bar-format-string " [%d of %d](%d%%%%)" ;how about using 
`format-spec'?
   "String for formatting the progress bar.
 Arguments passed are current-step, total-steps and completed percentage.
 Consider using field number arguments for more flexibility.
 See `format' documentation."
-  :type 'string
-  :group 'progress-bar)
+  :type 'string)
 
 (defcustom progress-bar-min-time 0.2
   "The minimum time interval between progress bar displays."
-  :type 'float
-  :group 'progress-bar)
+  :type 'number)
 
 (defcustom progress-bar-min-change 1
   "The minimum percentage change required between progress bar displays."
-  :type 'integer
-  :group 'progress-bar)
+  :type 'number)
 
 (defcustom progress-bar-message-display-layout
   'concatenate
@@ -109,15 +108,14 @@ If `dynamic', the message is either concatenated or 
inserted after a new line
 depending on its length."
   :type '(choice (const concatenate)
                  (const newline)
-                 (const dynamic))
-  :group 'progress-bar)
+                 (const dynamic)))
 
 (defvar progress-bar-update-functions '()
   "An abnormal hook for getting notified of progress bar updates.
 Functions get called with a progress bar event, and a progress-bar instance.
 Progress bar events can be either `started', `updated' or `completed'")
 
-(cl-defstruct progress-bar
+(cl-defstruct progress-bar             ;please address the `checkdoc' messages!
   (status-message nil
                   :documentation "The status-message can be either a 
status-formatter or a list of three status-formatters, the first applied when 
the progress-bar starts, the second applied for each element processed, the 
third when the progress-bar completes.
 A status-formatter is either a string or a function that takes a progress-bar 
instance and returns a string.")
@@ -159,7 +157,7 @@ See `progress-bar-update-functions' hook."
 ARGS is a property-list of slot-name and value.
 
 Example:
-(progress-bar-update pg 'current-step 2 'data 'foo)"
+\(progress-bar-update pg \\='current-step 2 \\='data \\='foo)"
   (cl-loop for (slot value) on args by 'cddr
            do (setf (slot-value progress-bar slot) value))
   (progress-bar--display progress-bar)
@@ -168,11 +166,12 @@ Example:
     (progress-bar-notify 'updated progress-bar)))
 
 (defun progress-bar-incf (progress-bar &optional increment display)
-  "Increment step in PROGRESS-BAR."
+  "Increment step by STEP in PROGRESS-BAR.
+If DISPLAY is non-nil, ..."
   (let ((inc (or increment 1)))
     (with-slots (current-step total-steps) progress-bar
       (when (and total-steps (> (+ current-step inc) total-steps))
-        (error "current-step > total-steps"))
+        (error "current-step > total-steps")) ;please rephrase this error 
message to be understandable on its own
       (cl-incf current-step inc)
       (when display
         (progress-bar--display progress-bar))
@@ -185,7 +184,7 @@ Example:
   (if (progress-bar-completed-p progress-bar)
       100
     (with-slots (current-step total-steps) progress-bar
-      (truncate (* (/ current-step (float total-steps)) 100)))))
+      (truncate (* current-step 100.0) total-steps))))
 
 (defun progress-bar--display (progress-bar)
   "Display PROGRESS-BAR in echo-area."
@@ -207,7 +206,7 @@ Example:
 
 (defun progress-bar--format-status-message (progress-bar message)
   (cl-etypecase message
-    ((or symbol function)
+    (function ;; you don't need to check symbols separately: (cl-typep 'insert 
'function) ;=> t
      (funcall message progress-bar))
     (string message)))
 
@@ -256,7 +255,7 @@ evaluating FUNC, so that messages are displayed together 
with the progress bar."
       (funcall func progress-bar)
     ;; Replace the implementation of `message' temporarily, so that
     ;; messages sent by FUNC are shown together with the progress bar.
-    (let ((emacs-message (symbol-function 'message)))
+    (let ((emacs-message (symbol-function #'message)))
       (cl-flet ((pb-message (msg &rest args)
                   ;; This is only for logging. Can we log the message
                   ;; without calling `message' ?
@@ -286,10 +285,10 @@ evaluating FUNC, so that messages are displayed together 
with the progress bar."
 
 (defmacro with-progress-bar (spec &rest body)
   "Create a PROGRESS-BAR binding SPEC in BODY scope.
-SPEC has either the form (VAR PROGRESS-BAR-INSTANCE) or (VAR &rest INITARGS), 
with
-INITARGS used for creating a `progress-bar'.
-This macros sets up special treatment for calls to MESSAGE that may ocurr in 
BODY,
-so that messages are displayed together with the progress bar."
+SPEC has either the form (VAR PROGRESS-BAR-INSTANCE) or (VAR &rest
+INITARGS), with INITARGS used for creating a `progress-bar'.  This
+macros sets up special treatment for calls to MESSAGE that may ocurr in
+BODY, so that messages are displayed together with the progress bar."
   (declare (indent 2))
   (cl-destructuring-bind (var &rest initargs) spec
     (if (= (length initargs) 1)
@@ -298,9 +297,9 @@ so that messages are displayed together with the progress 
bar."
       `(let ((,var (make-progress-bar ,@initargs)))
          (call-with-progress-bar ,var (lambda (,var) ,@body))))))
 
-;; Utilities
+;;;; Utilities
 
-(defun mapc-with-progress-bar (func sequence &rest args)
+(defun progress-bar-mapc (func sequence &rest args)
   "Like `mapc' but using a progress-bar."
   (let ((progress-bar (if (= (length args) 1)
                           (car args)
@@ -318,7 +317,7 @@ so that messages are displayed together with the progress 
bar."
       (progress-bar--display progress-bar)
       (progress-bar-notify 'completed progress-bar))))
 
-(defmacro dolist-with-progress-bar (spec &rest body)
+(defmacro progress-bar-dolist- (spec &rest body)
   "Like DOLIST but displaying a progress-bar as items in the list are 
processed.
 ARGS are arguments for `make-progress-bar'.
 
@@ -334,7 +333,7 @@ Example:
   (cl-destructuring-bind (var list &rest args) spec
     `(mapc-with-progress-bar (lambda (,var) ,@body) ,list ,@args)))
 
-(defmacro dotimes-with-progress-bar (spec &rest body)
+(defmacro progress-bar-dotimes (spec &rest body)
   "Like `dotimes' but with a progress bar."
   (declare (indent 2))
   (let ((progress-bar (gensym "progress-bar-")))


> progress-bar is a progress bar that is displayed in the echo area.
>
> Please let me know if you accept, and what would be the next steps.

My main question, which I realised too late when reading the code, is if
you could rework this to integrate into existing instances of
`make-progress-reporter', just replacing the UI.  It seems like it would
be more effective and consistent, and avoid hard dependencies of
programs that want to use `dotimes-with-progress-bar' (or as I renamed
it `progress-bar-dotimes' to avoid namespace clashes), when
`dotimes-with-progress-reporter' already exists and is being used.

> Thank you,
>
>      Mariano
>
>
>

-- 
        Philip Kaludercic on siskin