Re: [NonGNU ELPA] 11 new packages!

[Philip Kaludercic <philipk@posteo.net>]

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

Akib Azmain Turja <akib@disroot.org> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>>>> OK, then adding them to NonGNU ELPA seems like the safer bet.
>>>>
>>>> I'd like to add them, but I'll have to take the time to review them
>>>> first, which might take a bit.
>>>
>>> What do you want to review?  The patches, or the packages?
>>
>> The packages, unless you aren't interested in comments.
>
> Review if you wish, I welcome feedback.

I've managed to skim through workroom.el.  The code looks great, so I
just have a non-comprehensive list of comments and ideas:


[-- Attachment #2: Type: text/plain, Size: 6559 bytes --]

diff --git a/workroom.el b/workroom.el
index 7091645b85..8bb564e495 100644
--- a/workroom.el
+++ b/workroom.el
@@ -217,12 +217,13 @@ The value is a mode line terminal like `mode-line-format'."
     keymap)
   "Keymap containing all useful commands of Workroom.")
 
-(defvar workroom-mode-map (make-sparse-keymap)
+(defvar workroom-mode-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map workroom-command-map-prefix
+		workroom-command-map)
+    map)
   "Keymap for Workroom mode.")
 
-(define-key workroom-mode-map workroom-command-map-prefix
-            workroom-command-map)
-
 (defun workroom-rebind-command-map-prefix ()
   "Rebind command prefix key sequence `workroom-command-map-prefix'."
   (substitute-key-definition
@@ -234,6 +235,7 @@ The value is a mode line terminal like `mode-line-format'."
 ;;;; Workroom and View Manipulation.
 
 (cl-defstruct (workroom--room
+	       (:predicate workroomp)
                (:constructor workroom--make-room)
                (:copier workroom--copy-room))
   "Structure for workroom."
@@ -253,6 +255,7 @@ The value is a mode line terminal like `mode-line-format'."
    :documentation "`completing-read' history of view names."))
 
 (cl-defstruct (workroom--view
+	       (:predicate workroom-view-p)
                (:constructor workroom--make-view)
                (:copier workroom--copy-view))
   "Structure for view of workroom."
@@ -268,7 +271,7 @@ The value is a mode line terminal like `mode-line-format'."
 (defvar workroom--dont-clear-new-view nil
   "Non-nil mean don't clear empty new views.")
 
-(defvar workroom--rooms nil
+(defvar workroom--rooms nil		;maybe some comments on the structure
   "List of currently live workrooms.")
 
 (defvar workroom-room-history nil
@@ -283,10 +286,6 @@ that.")
 
 (defvar workroom-mode)
 
-(defun workroomp (object)
-  "Return non-nil if OBJECT is a workroom object."
-  (workroom--room-p object))
-
 (defun workroom-name (room)
   "Return the name of workroom ROOM."
   (workroom--room-name room))
@@ -384,10 +383,6 @@ effect, it is not unaltered."
   "Completing read history of view of workroom ROOM."
   (workroom--room-view-history room))
 
-(defun workroom-view-p (object)
-  "Return non-nil if OBJECT is a view object."
-  (workroom--view-p object))
-
 (defun workroom-view-name (view)
   "Return the name of view VIEW."
   (workroom--view-name view))
@@ -435,10 +430,9 @@ A copy is returned, so it can be modified with side-effects."
   "Return the workroom named NAME.
 
 If no such workroom exists, return nil."
-  (catch 'found
-    (dolist (room workroom--rooms nil)
-      (when (string= name (workroom-name room))
-        (throw 'found room)))))
+  (cl-find name workroom--rooms
+	   :key #'workroom-name
+	   :test #'string=))
 
 (defun workroom-get-create (name)
   "Return the workroom named NAME.
@@ -500,7 +494,7 @@ If no such view exists, create a new one named NAME and return that."
     (unless view
       (setq view (workroom--make-view :name name))
       (setf (workroom--room-view-list room)
-            (nconc (workroom--room-view-list room) `(,view))))
+            (nconc (workroom--room-view-list room) (list view))))
     view))
 
 (defun workroom-generate-new-view-name (room name)
@@ -516,7 +510,7 @@ name."
       (let ((n 2))
         (while t
           (let ((str (format "%s<%i>" name n)))
-            (when (not (workroom-view-get room str))
+            (unless (workroom-view-get room str)
               (cl-return str))
             (cl-incf n)))))))
 
@@ -558,7 +552,7 @@ Return DEF when input is empty, where DEF is either a string or nil.
 
 REQUIRE-MATCH and PREDICATE is same as in `completing-read'."
   (completing-read
-   (concat prompt (when def (format " (default %s)" def)) ": ")
+   (concat prompt (and def (format " (default %s)" def)) ": ") ;Compat has `format-prompt'
    (mapcar #'workroom-name workroom--rooms) predicate require-match
    nil 'workroom-room-history def))
 
@@ -670,7 +664,7 @@ If WRITABLE, return a writable object."
 (defun workroom--load-window-config (state)
   "Load window configuration STATE."
   (if state
-      (cl-labels
+      (cl-labels			;perhaps this should be split up?
           ((sanitize (entry)
              (cond
               ;; Do nothing.
@@ -1254,7 +1248,7 @@ ACTION and ARGS are also described there."
   (setf (workroom-buffer-manager-data room)
         (cl-delete-if-not #'buffer-live-p
                           (workroom-buffer-manager-data room)))
-  (pcase action
+  (pcase action				;perhaps match on (cons action args)?
     (:initialize
      (cl-destructuring-bind () args
        (setf (workroom-buffer-manager-data room)
@@ -1507,9 +1501,14 @@ restrict."
 
 (defun workroom--encode-view-1 (view)
   "Encode view VIEW to a writable object."
-  `( :name ,(workroom-view-name view)
-     :window-config ,(workroom-view-window-configuration
-                      view 'writable)))
+  ;; I think it is preferable to define plists with keywords using
+  ;; `list', as all the members of the list can be evaluated during
+  ;; application.
+  (list :name
+	(workroom-view-name view)
+	:window-config
+	(workroom-view-window-configuration
+	 view 'writable)))
 
 (defun workroom--decode-view-1 (object)
   "Decode encoded view OBJECT to a view."
@@ -1589,7 +1588,7 @@ when ROOM was encoded."
 
 (defun workroom-decode-buffer-bookmark (object)
   "Decode OBJECT using `bookmark-jump'."
-  (let* ((buffer nil))
+  (let ((buffer nil))
     (bookmark-jump object (lambda (buf) (setq buffer buf)))
     buffer))
 
@@ -1664,7 +1663,7 @@ any previous bookmark with the same name."
                                      bookmark)))))
     (pcase (plist-get data :version)
       (1
-       (let* ((buffers (cl-delete-if
+       (let ((buffers (cl-delete-if
                         #'null
                         (workroom--decode-buffers
                          (plist-get data :buffers)))))
@@ -1787,6 +1786,7 @@ any previous bookmark with the same name."
   ;; Inject restoring code.
   (when workroom-mode
     (let ((time (format-time-string "%s%N")))
+      ;; I like to use `prin1-to-string' for things like these.
       (insert
        (format
         "
@@ -1951,6 +1951,7 @@ argument while setting as the buffer manager, PROJECT, the project."
 (defun workroom--project-name (project)
   "Return a name for project PROJECT."
   (let ((root (project-root project)))
+    ;; Isn't this `file-name-base' or `directory-file-name'?
     (if (string-match "/\\([^/]+\\)/?\\'" root)
         (match-string 1 root)
       root)))