From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Philip Kaludercic Newsgroups: gmane.emacs.devel Subject: Re: [NonGNU ELPA] 11 new packages! Date: Mon, 21 Nov 2022 18:32:29 +0000 Message-ID: <87wn7o9n0i.fsf@posteo.net> References: <87r0y6ug9z.fsf@disroot.org> <87y1sct2hp.fsf@posteo.net> <87k03vf5m8.fsf@disroot.org> <87edu2narn.fsf@posteo.net> <8735aieqtr.fsf@disroot.org> <87cz9mlq3o.fsf@posteo.net> <875yfdd5ad.fsf@disroot.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="2336"; mail-complaints-to="usenet@ciao.gmane.io" Cc: Emacs Developer List To: Akib Azmain Turja Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Mon Nov 21 19:33:42 2022 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oxBbx-0000Mk-NC for ged-emacs-devel@m.gmane-mx.org; Mon, 21 Nov 2022 19:33:41 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oxBav-0005A9-9L; Mon, 21 Nov 2022 13:32:37 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oxBat-00059c-Ao for emacs-devel@gnu.org; Mon, 21 Nov 2022 13:32:35 -0500 Original-Received: from mout01.posteo.de ([185.67.36.65]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oxBaq-0000HF-L4 for emacs-devel@gnu.org; Mon, 21 Nov 2022 13:32:35 -0500 Original-Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id 33112240028 for ; Mon, 21 Nov 2022 19:32:29 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1669055549; bh=cBWkZ3FDzKxKTwkOzLXwTs6WL2Jv5WlqQBWVR5pYFyg=; h=From:To:Cc:Subject:Date:From; b=cvU0wv3VD98+mcOlTu7f6AONWYN17KxnZGWUW5N995Q3M6bmTyS3QpyJA8WWY9oBw CBI8Tv/ODBVrczWImwapRzb1f8nnE68JZEFbeRcJR8Xa0mnbbVC2LNXBAj3kZTS59U v77bOes+Xvy3E0ziowmaX9IAR3gAM4gfVXSI542kpU6eXlr2useCXz5vQcEO3hYiRZ aKwDp9HAZMsBXJl9FE35VIH5QDylrcb+nyPh6QfbrPUhdTrkZaNlBZWuW6K3tcqJPu 7GYmB6nlVa9EtO+1kE9hAzEp7HE8jepuep7zULCm93o7icEUZZ+0GuIFT4e5GT4uiN trYcTiDPcz/CQ== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4NGGG44jFHz6tnX; Mon, 21 Nov 2022 19:32:28 +0100 (CET) In-Reply-To: <875yfdd5ad.fsf@disroot.org> (Akib Azmain Turja's message of "Thu, 17 Nov 2022 20:28:10 +0600") Received-SPF: pass client-ip=185.67.36.65; envelope-from=philipk@posteo.net; helo=mout01.posteo.de X-Spam_score_int: -43 X-Spam_score: -4.4 X-Spam_bar: ---- X-Spam_report: (-4.4 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_MED=-2.3, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:300305 Archived-At: --=-=-= Content-Type: text/plain Akib Azmain Turja writes: > Philip Kaludercic 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: --=-=-= Content-Type: text/plain Content-Disposition: inline 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))) --=-=-=--