Re: [ELPA] New package: project-shells

[Thien-Thi Nguyen <ttn@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 4032 bytes --]


() "Huang, Ying" <huang_ying_caritas@163.com>
() Fri, 03 Mar 2017 20:46:40 +0800

   ;; Version: 20170222
   ;; Package-Version: 20170222

Reminder: Don't forget to update these fields.

     "Manage shell buffers of each project"

Needs period (?., U+2E) at end.  Similarly for other instances.

   type ('shell or 'term)

This is slightly unconventional.  You explicitly name the type
of the other alist components, why not this one as well?  Maybe:

 TYPE (a symbol, either ‘shell’ or ‘term’)

Another convention is to use all uppercase for metavariables.
For example, see ‘auto-mode-alist’.  (Thus, "TYPE" above.)

   One shell will be created for each key.  Usually these key will
   be bound in a non-global keymap."

I suggest naming the function that does this action.  For
example, "Normally, ‘project-shells-setup’ arranges for these
keys to open a project-specific shell.".  Naming the function
(w/ proper quotes) creates a hyperlink in the *Help* buffer that
helps the reader form a mental model of the package operation.
The "Normally" serves two purposes:

 (a) It sidesteps the decision to choose between capitalizing
     "project-shells-setup", the first word in the sentence
     (correct from a grammar pov) and leaving it as-is (correct
     from an accuracy pov).

 (b) It invites follow-on documentation for any specific
     customization tips.

I think (a) is a nice (technical writing) hack only, but (b) is
crucial to the user experience (and maintenance burden) of this
package.

   (defcustom project-shells-term-keys '("-" "=")
     "Keys used to create terminal buffers.

   By default shell mode will be used, but for keys in
   ‘project-shells-term-keys’, ansi terminal mode will be used.  This
   should be a subset of *poject-shells-keys*."

Spelling error: "poject".  Also, don't use asterisk (?*, U+2A)
to quote API elements.  Instead, use either backtick (?`, U+60)
and apostrophe (?', U+27) or Unicode chars LEFT SINGLE QUOTATION
MARK (?‘, U+2018) and RIGHT SINGLE QUOTATION MARK (?’, U+2019).

Lastly, design questions (rhetorical): What happens if "should
be a subset" condition does not hold?  Have you considered
combining ‘project-shells-term-keys’ and ‘project-shell-keys’
somehow?  What if my project supports live-hacking via REPL?

   (let ((saved-shell-buffer-list nil)
         (last-shell-name nil))

     (cl-defun project-shells--buffer-list ...)

     (cl-defun project-shells--switch ...)

     (cl-defun project-shells-switch-to-last ...)

     (cl-defun project-shells--create ...))

Nice, much less scary for old eyes to read now.  (I added
another blank line prior to ‘project-shells--buffer-list’, btw.)

   (cl-defun project-shells-send-shell-command (cmdline)
     "Send the command line to the current (shell) buffer.  Can be
   used in shell initialized function."

Move the "Can be..." sentence to the next line.  More tips at:
(info "(elisp) Documentation Tips")

     (let* ((key (replace-regexp-in-string "/" "slash" key))
            ...
            (session-dir (expand-file-name (format "%s/%s" proj key)
                                           project-shells-session-root)))

This is is why (b) above is important.  This code special-cases
slash (?/, U+2F), but will probably give surprising results for
‘\M-g’ or ‘\C-/’ and so on.  You can either add handling for
those here, or document the range of "acceptable" ‘key’ values.
If you don't, you will receive complaints from users who try to
use project-shells.el w/ strange (but valid) keys.  I don't know
about you, but i get enough complaints as it is...  :-D

-- 
Thien-Thi Nguyen -----------------------------------------------
 (defun responsep (query)
   (pcase (context query)
     (`(technical ,ml) (correctp ml))
     ...))                              748E A0E8 1CB8 A748 9BFA
--------------------------------------- 6CE4 6703 2224 4C80 7502


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]