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.
RetroSearch is an open source project built by @garambo | Open a GitHub Issue
Search and Browse the WWW like it's 1997 | Search results from DuckDuckGo
HTML:
3.2
| Encoding:
UTF-8
| Version:
0.7.4