all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Philip Kaludercic <philipk@posteo.net>
To: Akib Azmain Turja <akib@disroot.org>
Cc: Emacs Developer List <emacs-devel@gnu.org>
Subject: Re: [NonGNU ELPA] 11 new packages!
Date: Tue, 22 Nov 2022 17:07:00 +0000	[thread overview]
Message-ID: <87r0xuapfv.fsf@posteo.net> (raw)
In-Reply-To: <87cz9fyq1g.fsf@disroot.org> (Akib Azmain Turja's message of "Tue, 22 Nov 2022 21:20:11 +0600")

Akib Azmain Turja <akib@disroot.org> writes:

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

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.



  reply	other threads:[~2022-11-22 17:07 UTC|newest]

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-14  7:42 [NonGNU ELPA] 11 new packages! Akib Azmain Turja
2022-11-15 17:42 ` Akib Azmain Turja
2022-11-15 19:53 ` Filipp Gunbin
2022-11-16 12:22   ` Akib Azmain Turja
2022-11-16 16:53     ` Eli Zaretskii
2022-11-16 17:43       ` Akib Azmain Turja
2022-11-16 19:47         ` Eli Zaretskii
2022-11-17 14:27           ` Akib Azmain Turja
2022-11-17 18:41             ` Stefan Monnier
2022-11-17 18:51             ` Yuan Fu
2022-11-24 23:38               ` Richard Stallman
2022-11-17 18:56             ` Eli Zaretskii
2022-11-15 19:57 ` Philip Kaludercic
2022-11-16 12:25   ` Akib Azmain Turja
2022-11-16 16:07     ` Philip Kaludercic
2022-11-16 17:45       ` Akib Azmain Turja
2022-11-16 18:19         ` Philip Kaludercic
2022-11-17 14:28           ` Akib Azmain Turja
2022-11-21 18:32             ` Philip Kaludercic
2022-11-22 15:20               ` Akib Azmain Turja
2022-11-22 17:07                 ` Philip Kaludercic [this message]
2022-11-22 17:42                   ` Akib Azmain Turja
2022-11-25  8:29                     ` Akib Azmain Turja
2022-11-25 16:32                       ` Philip Kaludercic
2022-11-25 17:14                         ` Akib Azmain Turja
2022-11-22 21:16                 ` Stefan Monnier
2022-11-25  7:14             ` Philip Kaludercic
2022-11-25  7:22               ` Philip Kaludercic
2022-11-25 12:45                 ` Akib Azmain Turja
2022-11-25 13:07               ` Akib Azmain Turja
2022-11-25 17:16                 ` Akib Azmain Turja
2022-11-25 17:31                 ` Philip Kaludercic
2022-11-26  5:53                   ` Akib Azmain Turja
2022-11-26 20:12                     ` Stefan Monnier
2022-11-25 19:07             ` Philip Kaludercic
2022-11-26  7:49               ` Akib Azmain Turja
2022-11-27  8:11                 ` Philip Kaludercic
2022-11-27 19:22                   ` Akib Azmain Turja
2022-11-27 19:55                   ` Akib Azmain Turja
2022-11-27 20:30                     ` Philip Kaludercic
2022-11-26 18:44               ` Akib Azmain Turja
2022-11-26 20:07             ` Philip Kaludercic
2022-11-26 20:17               ` Philip Kaludercic
2022-11-26 21:12                 ` Akib Azmain Turja
2022-11-27 11:45                   ` Philip Kaludercic
2022-11-27 17:04                     ` Akib Azmain Turja
2022-11-27 20:26                       ` Philip Kaludercic
2022-11-28 19:07                       ` Akib Azmain Turja
2022-11-28 19:42                         ` Philip Kaludercic
2022-11-28 20:32                           ` Akib Azmain Turja
2022-11-28 20:57                         ` Stefan Monnier
2022-11-27 18:40                   ` [NonGNU ELPA] 12 " Akib Azmain Turja
2022-11-27 20:36                     ` Philip Kaludercic
2022-11-27 21:11                       ` Akib Azmain Turja
2022-11-27  6:40               ` [NonGNU ELPA] 11 " Akib Azmain Turja
2022-11-19 12:05     ` Richard Stallman
2022-11-19 12:17       ` Philip Kaludercic

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87r0xuapfv.fsf@posteo.net \
    --to=philipk@posteo.net \
    --cc=akib@disroot.org \
    --cc=emacs-devel@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.