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: Tue, 22 Nov 2022 17:07:00 +0000 Message-ID: <87r0xuapfv.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> <87wn7o9n0i.fsf@posteo.net> <87cz9fyq1g.fsf@disroot.org> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="34351"; 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 Tue Nov 22 18:07: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 1oxWkU-0008lg-VQ for ged-emacs-devel@m.gmane-mx.org; Tue, 22 Nov 2022 18:07:55 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oxWjo-0005i8-DC; Tue, 22 Nov 2022 12:07:12 -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 1oxWjl-0005hX-BR for emacs-devel@gnu.org; Tue, 22 Nov 2022 12:07:09 -0500 Original-Received: from mout02.posteo.de ([185.67.36.66]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oxWji-0007dh-7g for emacs-devel@gnu.org; Tue, 22 Nov 2022 12:07:08 -0500 Original-Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id 56D53240101 for ; Tue, 22 Nov 2022 18:07:00 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1669136823; bh=8cJqQxqvcKPZbDbOu+TLUeTJ9aAX9funXc5MPQ3zOps=; h=From:To:Cc:Subject:Date:From; b=e8T0vKfKC7d0O9xtFY3M/pGnA+hpJTUOAOukFmlN888u32M2W7pin6rhwWbY0G77J 3prLaRSsqkyZkgkN/DQfdJgwiu0QoStLCz5Wf1p6LZBf1V6VwkC1nVd4bk16w157bY bAedOEOhtS0zByAPWvuj/3Z8oSYw2n+uYFDWSRQvFLDXNTPCy4HrDhAfnnYmNvwou2 /wuxdqAorMlADCNIia11K5dUDtzqKuw4bf7LS5AAf2bDYe1HePUGPqPPmX6jLmvTOZ NtLrUzPUThqmXHEwjH5RxU80Gzp/EHQvSYNt1ffMV9JQnPiK6YLOcwd40kTeeEkm0o 23EqwMCj2Xi6Q== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4NGrK01kbYz6tnG; Tue, 22 Nov 2022 18:07:00 +0100 (CET) In-Reply-To: <87cz9fyq1g.fsf@disroot.org> (Akib Azmain Turja's message of "Tue, 22 Nov 2022 21:20:11 +0600") Received-SPF: pass client-ip=185.67.36.66; envelope-from=philipk@posteo.net; helo=mout02.posteo.de X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 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_LOW=-0.7, RCVD_IN_MSPIKE_H2=-0.001, 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:300339 Archived-At: Akib Azmain Turja writes: > 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. The coding style was clean and the documentation went into details. That is good stuff in my book :) >> (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. :( That might be the case. In that case you can keep the previous code, and perhaps define the prettier variants using `defalias`, or by manually annotating a function symbol. >> >> -(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'. Hmm, I guess that is self-descriptive enough. I was wondering if this was a alist or something else. >> @@ -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? Compat is provided on ELPA. If you add it as a dependency and load it, it will define missing functionality on older systems. The documentation goes into greater detail: https://elpa.gnu.org/packages/doc/compat.html#Usage. But as everything else, this is just a "fun fact", nothing critical. >> @@ -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 I assumed this was just for the sake of consistency? After all, my suggestion does do one unnecessary `cons' for the sake of convenience.