Re: [NonGNU ELPA] 11 new packages!

[Akib Azmain Turja <akib@disroot.org>]

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

Philip Kaludercic <philipk@posteo.net> writes:

> 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:

Really?  It was one of my worst packages until I almost rewrote it over
the last few weeks.

>
> 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)
> -

I just removed that define-key completely.  `workroom-mode` will bind it
when enabled.

>  (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.")

They don't get a docstring on my machine.  :(

>  
> -(defvar workroom--rooms nil
> +(defvar workroom--rooms nil		;maybe some comments on the structure
>    "List of currently live workrooms.")

As the docstring describes, it's a list, and all elements satisfies
`workroomp'.

>  
>  (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))
> -

I just wanted to give it a better docstring.  ;)

>  (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=))

Done, also changed workroom-view-get.

>  
>  (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))

Done.

>  
>  (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)))))))

I can't believe I wrote (when (not ...) ...)!

>  
> @@ -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))

I don't have much idea about Compat, how does it work?

>  
> @@ -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.

Yeah, it's too monolithic.

> @@ -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)

I wonder why I used cl-destructuring-bind when there isn't any keyword
arguments.

Fixing...
Fixing...done

> @@ -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)))
>  

Done.  Maybe I did too much.

>  (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)))))

Ah, I wasted two bytes for each copy of Workroom worldwide.  :(
Fixed.

> @@ -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
>          "

Done, and I got syntax highlighting as a bonus!

> @@ -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)))
>

Yeah, my bad, it's actually `file-name-base'.  I threw the function
away.

-- 
Akib Azmain Turja, GPG key: 70018CE5819F17A3BBA666AFE74F0EFA922AE7F5
Fediverse: akib@hostux.social
Codeberg: akib
emailselfdefense.fsf.org | "Nothing can be secure without encryption."

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]