From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Thien-Thi Nguyen Newsgroups: gmane.emacs.devel Subject: Re: [ELPA] New package: project-shells Date: Sat, 25 Feb 2017 09:46:36 +0100 Message-ID: <87varyd5eb.fsf@zigzag.favinet> References: <87mvdbojds.fsf@163.com> Reply-To: emacs-devel@gnu.org NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" X-Trace: blaine.gmane.org 1488012664 1611 195.159.176.226 (25 Feb 2017 08:51:04 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sat, 25 Feb 2017 08:51:04 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.0.50 (gnu/linux) Cc: emacs-devel@gnu.org To: "Huang\, Ying" Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sat Feb 25 09:50:58 2017 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1chY47-00084a-Qs for ged-emacs-devel@m.gmane.org; Sat, 25 Feb 2017 09:50:56 +0100 Original-Received: from localhost ([::1]:41827 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1chY4D-0002Js-Ev for ged-emacs-devel@m.gmane.org; Sat, 25 Feb 2017 03:51:01 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:55639) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1chY3d-0002Jl-UJ for emacs-devel@gnu.org; Sat, 25 Feb 2017 03:50:27 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1chY3a-0000Iq-Sk for emacs-devel@gnu.org; Sat, 25 Feb 2017 03:50:26 -0500 Original-Received: from mail.agora-net.com ([67.59.132.6]:50524) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1chY3a-0000IW-Oc for emacs-devel@gnu.org; Sat, 25 Feb 2017 03:50:22 -0500 Original-Received: from ttn by mail.agora-net.com with local (Exim 4.82) (envelope-from ) id 1chY3W-0000nS-8E; Sat, 25 Feb 2017 03:50:18 -0500 Original-Received: from ttn by zigzag.favinet with local (Exim 4.80) (envelope-from ) id 1chY07-0004Bg-5q; Sat, 25 Feb 2017 09:46:47 +0100 Mail-Followup-To: emacs-devel@gnu.org In-Reply-To: <87mvdbojds.fsf@163.com> (Ying Huang's message of "Fri, 24 Feb 2017 20:36:47 +0800") X-SA-Exim-Connect-IP: X-SA-Exim-Mail-From: ttn@gnuvola.org X-SA-Exim-Scanned: No (on mail.agora-net.com); SAEximRunCond expanded to false X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 67.59.132.6 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 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.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:212589 Archived-At: --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable () "Huang, Ying" () Fri, 24 Feb 2017 20:36:47 +0800 ;;; Commentary: ;; [...] (require 'cl-lib) This is missing ";;; Code:" prior to the =E2=80=98require=E2=80=99 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 =E2=80=98s/Setup/Default configuration form/=E2=80=99 = 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 =E2=80=98defcustom=E2=80=99 and =E2=80=98project-s= hells-setup=E2=80=99. (let ((saved-shell-buffer-list nil) (last-shell-name)) Both =E2=80=98saved-shell-buffer-list=E2=80=99 and =E2=80=98last-shell-name= =E2=80=99 have initial binding =E2=80=98nil=E2=80=99. 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 =E2=80=98cl-defun=E2=80=99 forms would he= lp 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 =E2=80=98func=E2=80=99 (if, when, h= ow 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 =E2=80=98cl-flet*=E2=80=99 to incorporate the first three into the la= st? (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 =E2=80=98key=E2=80=99 is slash (or backslash, =E2=80= =98M-g=E2=80=99, ...)? (format "%s/%s" session-dir project-shells-histfile-name) More idiomatic to use =E2=80=98expand-file-name=E2=80=99 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 (/=3D p 1) project-shells-empty-project))))))) Have you considered using =E2=80=98this-command-keys=E2=80=99 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 =2D-=20 Thien-Thi Nguyen ----------------------------------------------- (defun responsep (query) (pcase (context query) (`(technical ,ml) (correctp ml)) ...)) 748E A0E8 1CB8 A748 9BFA =2D-------------------------------------- 6CE4 6703 2224 4C80 7502 --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) iEYEARECAAYFAlixRHAACgkQZwMiJEyAdQKUeQCeOguI8sv4Fi7SGS2vtJiFkTew NiMAn2wWlhQL71+KGL8xPkDRvLQAKo9X =o6Tv -----END PGP SIGNATURE----- --=-=-=--