From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Akib Azmain Turja Newsgroups: gmane.emacs.devel Subject: Re: [NonGNU ELPA] 11 new packages! Date: Tue, 22 Nov 2022 21:20:11 +0600 Message-ID: <87cz9fyq1g.fsf@disroot.org> 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> <87wn7o9n0i.fsf@posteo.net> Mime-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="7981"; mail-complaints-to="usenet@ciao.gmane.io" Cc: Emacs Developer List To: Philip Kaludercic Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Tue Nov 22 16:22:55 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 1oxV6r-0001la-NL for ged-emacs-devel@m.gmane-mx.org; Tue, 22 Nov 2022 16:22:53 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oxV5e-0007eB-Uc; Tue, 22 Nov 2022 10:21:38 -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 1oxV5a-0007Xe-2W for emacs-devel@gnu.org; Tue, 22 Nov 2022 10:21:34 -0500 Original-Received: from knopi.disroot.org ([178.21.23.139]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oxV5X-0004z7-W3 for emacs-devel@gnu.org; Tue, 22 Nov 2022 10:21:33 -0500 Original-Received: from localhost (localhost [127.0.0.1]) by disroot.org (Postfix) with ESMTP id B694B413A3; Tue, 22 Nov 2022 16:21:30 +0100 (CET) X-Virus-Scanned: SPAM Filter at disroot.org Original-Received: from knopi.disroot.org ([127.0.0.1]) by localhost (disroot.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id wTQsf7ETYeQR; Tue, 22 Nov 2022 16:21:27 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=disroot.org; s=mail; t=1669130487; bh=RbBVe+84gCV64YTAMDOXdq/tL7yK8lFxeh+IKiHi2CQ=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=gB9NetIaxKyMnRFzsHKdZ0vL92fVnbnE25rKNwUwJ6Rrtn7vnNR0vCIclJZNWfdyN vrofjuJl7mb9G79Ry143GDhMHu7lP5yiaZ6QueKGbC0oAEv/3vZN2kkDQQQrRpQXJU asfl+YYoUZtO27/jaB7U8R9FaqXrtEWRzwCM317BnuOykCBpyYqr9HBoG3G67cLgsi H5LMXxedUsgdKkqbMHdBzafknf6XrJZcWDGtnHVSKIf0Ee+gO/BO7JwtdVoGqY+QrG RTZsWnUbze+47/s024zWprlyl/3hdNjIrMQXswfvThQ8XFbNHJ3R2qowmLRE/L8lUW aQspJiliE0pTw== In-Reply-To: <87wn7o9n0i.fsf@posteo.net> (Philip Kaludercic's message of "Mon, 21 Nov 2022 18:32:29 +0000") Received-SPF: pass client-ip=178.21.23.139; envelope-from=akib@disroot.org; helo=knopi.disroot.org X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 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, 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:300335 Archived-At: --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Philip Kaludercic writes: > 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: 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-f= ormat'." > keymap) > "Keymap containing all useful commands of Workroom.") >=20=20 > -(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.") >=20=20 > -(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-for= mat'." > ;;;; Workroom and View Manipulation. >=20=20 > (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-for= mat'." > :documentation "`completing-read' history of view names.")) >=20=20 > (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-for= mat'." > (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. :( >=20=20 > -(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'. >=20=20 > (defvar workroom-room-history nil > @@ -283,10 +286,6 @@ that.") >=20=20 > (defvar workroom-mode) >=20=20 > -(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)) >=20=20 > -(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. >=20=20 > If no such workroom exists, return nil." > - (catch 'found > - (dolist (room workroom--rooms nil) > - (when (string=3D name (workroom-name room)) > - (throw 'found room))))) > + (cl-find name workroom--rooms > + :key #'workroom-name > + :test #'string=3D)) Done, also changed workroom-view-get. >=20=20 > (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 a= nd 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. >=20=20 > (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 ...) ...)! >=20=20 > @@ -558,7 +552,7 @@ Return DEF when input is empty, where DEF is either a= string or nil. >=20=20 > 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 h= as `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? >=20=20 > @@ -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." >=20=20 > (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))) >=20=20 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." >=20=20 > (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)) >=20=20 > @@ -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, PROJE= CT, 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. =2D-=20 Akib Azmain Turja, GPG key: 70018CE5819F17A3BBA666AFE74F0EFA922AE7F5 Fediverse: akib@hostux.social Codeberg: akib emailselfdefense.fsf.org | "Nothing can be secure without encryption." --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEyVTKmrtL6kNBe3FRVTX89U2IYWsFAmN86KwACgkQVTX89U2I YWvHZBAA5O1zas5uqj3vCeof4KrSQo2uZwOOkreVNIMLfuFvourcyCCAqndHTERH ey/xoHCtCxRHsS5awQOv/bV/hGLuW8tBZbf+6N64ULMn1/A8+lo+FZp8AZODMnAb ytSekbiew0x2yWAYNunE9Qlx6fOfLfcWWUOzd8fRAw7VW5WgSNaGBZWVXDW1gM4u Rwp2XFhi+Ef67IT2fOCMXkQXUGhbqUiIo9qxdSLPVnEsgxhWibAuG/1LkXqlpty6 kuYlV521enHhBtMGgEi2gdVkOPXnB5nYN0yyfgwBWkdS0Nt29Td7pV8QkfotM+sc zgESMaHVEuDcdQxrHz4wYTAxsK9sUkF6xlRQnJQRYUtZuxcNYuRmo/H6kx+V3pkm f4nqMycn4JbCe5QqLPqUdi5BSQl8l+yC6oqzffpi4F61zzyZyQmB2IjjMZe+u7kc kecX/5eK4vdkpjqg7m21D66WZI2qoq0WdTe1gYQiphfT3MzeOHugIf3KuXwepWZ0 h5SvlDzQNNNJaTnVaXnjXl85Qni7rbTRBpYuLQ4nGi+Nk/2kQ5zPqn2Q5fRQz5rs QU1EWU0F5KKP0VHjG5xr7C+yc4vIgr85eLOwHHOJVqnZcxI9unLCd2ZQsx54LDr8 E6zyepRJpqYWnfLmApW9ygfSLVOGdbPsNrskmEbW8Vw1j7GUg50= =E08W -----END PGP SIGNATURE----- --=-=-=--