Re: [ELPA] New package: project-shells

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

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


() "Huang, Ying" <huang_ying_caritas@163.com>
() Fri, 24 Feb 2017 20:36:47 +0800

   ;;; Commentary:

   ;; [...]

   (require 'cl-lib)

This is missing ";;; Code:" prior to the ‘require’ form.

     "Specify the name of the empty project.
     "Specify the setup for shells of each project.
     "Specify keys for shells, one shell will be created for each key.
     [and so on]

These docstrings can be improved mostly by dropping the
"specify" and (perhaps) the article.  For example:

     "Name of the empty project.
     "Default configuration form for shells of each project.
     "Keys used for shells.
     [and so on]

In the second one, i ‘s/Setup/Default configuration form/’ and
suggest you describe form's format in the docstring.  In the
last one, i suggest you find a more descriptive term than
"used".  Also, i dropped the latter half of the run-on sentence.
You can add that back on the next line as a new sentence.

   (defcustom  project-shells-setup

Nit: extra space between ‘defcustom’ and ‘project-shells-setup’.

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

Both ‘saved-shell-buffer-list’ and ‘last-shell-name’ have
initial binding ‘nil’.  However, this is expressed in two
different ways (both valid).  Why?

     (cl-defun shell-buffer-list ()
       (setf saved-shell-buffer-list
             (cl-remove-if-not #'buffer-live-p saved-shell-buffer-list)))

Nit: Some blank lines between the ‘cl-defun’ forms would help
readability, especially important as these are not at top-level.

     (cl-defun project-shells-switch (&optional name to-create)
       (interactive "bShell: ")

All commands should have a docstring (even generated ones).

         (cl-ecase type
           ('term (ansi-term "/bin/sh"))
           ('shell (shell)))

Is there a reason you quote the key in the cl-ecase KEYLIST?
(Just curious.)

   (cl-defun project-shells-create-switch (name dir &optional (type 'shell) func)
     (unless (project-shells-switch name t)
       (project-shells-create name dir type func)))

Needs documentation, especially for arg ‘func’ (if, when, how it
is called; what happens if it is ill-formed or throws error, etc).

   (cl-defun project-shells-escape-sh (str) ...)

   (cl-defun project-shells-command-string (args) ...)

   (cl-defun project-shells-term-command-string () ...)

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

Each of these is called solely by the next.  Have you considered
using ‘cl-flet*’ to incorporate the first three into the last?

            (session-dir (expand-file-name (format "~/.sessions/%s/%s" proj key)))

This directory needs to be documented.  It would be nice if it
could be made customizable (e.g., to save under ~/.emacs.d/).
Also, what happens if ‘key’ is slash (or backslash, ‘M-g’, ...)?

        (format "%s/%s" session-dir project-shells-histfile-name)

More idiomatic to use ‘expand-file-name’ here.

   (cl-defun project-shells-setup (map &optional setup)
     (when setup
       (setf project-shells-setup setup))
     (cl-loop
      for key in project-shells-keys
      do (define-key map (kbd key)
           (let* ((key key))
             (lambda (p)
               (interactive "p")
               (project-shells-activate
                key (and (/= p 1) project-shells-empty-project)))))))

Have you considered using ‘this-command-keys’ and a single
command definition instead of defining a command per key?
Also, needs documentation.

One way to sidestep the need for documentation is to distinguish
between "private" and "public" elements, conventionally by using
a double-hyphen for internal (private) names.  Another way is to
incorporate single-caller funcs into that caller (as i mentioned
above).  Yet another (most poor IMHO) way is to never share your
code, but i'm very happy to see you've not chosen that way.  :-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 --]