[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [NonGNU ELPA] 11 new packages!
From: |
Philip Kaludercic |
Subject: |
Re: [NonGNU ELPA] 11 new packages! |
Date: |
Mon, 21 Nov 2022 18:32:29 +0000 |
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:
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)))
Re: [NonGNU ELPA] 11 new packages!, Philip Kaludercic, 2022/11/15
- Re: [NonGNU ELPA] 11 new packages!, Akib Azmain Turja, 2022/11/16
- Re: [NonGNU ELPA] 11 new packages!, Philip Kaludercic, 2022/11/16
- Re: [NonGNU ELPA] 11 new packages!, Akib Azmain Turja, 2022/11/16
- Re: [NonGNU ELPA] 11 new packages!, Philip Kaludercic, 2022/11/16
- Re: [NonGNU ELPA] 11 new packages!, Akib Azmain Turja, 2022/11/17
- Re: [NonGNU ELPA] 11 new packages!,
Philip Kaludercic <=
- Re: [NonGNU ELPA] 11 new packages!, Akib Azmain Turja, 2022/11/22
- Re: [NonGNU ELPA] 11 new packages!, Philip Kaludercic, 2022/11/22
- Re: [NonGNU ELPA] 11 new packages!, Akib Azmain Turja, 2022/11/22
- Re: [NonGNU ELPA] 11 new packages!, Akib Azmain Turja, 2022/11/25
- Re: [NonGNU ELPA] 11 new packages!, Philip Kaludercic, 2022/11/25
- Re: [NonGNU ELPA] 11 new packages!, Akib Azmain Turja, 2022/11/25
Re: [NonGNU ELPA] 11 new packages!, Stefan Monnier, 2022/11/22
Re: [NonGNU ELPA] 11 new packages!, Philip Kaludercic, 2022/11/25
Re: [NonGNU ELPA] 11 new packages!, Philip Kaludercic, 2022/11/25
Re: [NonGNU ELPA] 11 new packages!, Akib Azmain Turja, 2022/11/25