From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: [elpa] master 74818d5: Brief Mode v5.86 release. Date: Fri, 19 Oct 2018 11:35:29 -0400 Message-ID: References: <20181018141131.22680.53035@vcs0.savannah.gnu.org> <20181018141132.6E8BA208EC@vcs0.savannah.gnu.org> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Trace: blaine.gmane.org 1539963366 17816 195.159.176.226 (19 Oct 2018 15:36:06 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 19 Oct 2018 15:36:06 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux) To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Oct 19 17:36:02 2018 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 1gDWoj-0004WY-Rs for ged-emacs-devel@m.gmane.org; Fri, 19 Oct 2018 17:36:02 +0200 Original-Received: from localhost ([::1]:51145 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gDWqq-0003cL-6U for ged-emacs-devel@m.gmane.org; Fri, 19 Oct 2018 11:38:12 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:46703) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gDWq8-0003G9-DL for emacs-devel@gnu.org; Fri, 19 Oct 2018 11:37:30 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gDWq4-0000tj-BW for emacs-devel@gnu.org; Fri, 19 Oct 2018 11:37:28 -0400 Original-Received: from [195.159.176.226] (port=57733 helo=blaine.gmane.org) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1gDWq2-0000pO-BZ for emacs-devel@gnu.org; Fri, 19 Oct 2018 11:37:23 -0400 Original-Received: from list by blaine.gmane.org with local (Exim 4.84_2) (envelope-from ) id 1gDWns-0003W5-Uo for emacs-devel@gnu.org; Fri, 19 Oct 2018 17:35:08 +0200 X-Injected-Via-Gmane: http://gmane.org/ Original-Lines: 215 Original-X-Complaints-To: usenet@blaine.gmane.org Cancel-Lock: sha1:jkzp7zm63cbD8DzKStbTzmuyjk4= X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 195.159.176.226 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:230495 Archived-At: Ping? Stefan PS: In the mean time, I had to remove b.el because its lack of copyright header prevented the GNU ELPA scripts from building the package. Stefan Monnier writes: > Hi Luke, > > Thanks for working on Brief mode. I have some comments on your patch, tho. > >> +BRIEFVERSION=${BRIEFVERSION-"5.86"} > [...] >> + BRIEFVERSION ELPA brief version, default "5.86" > > I'd recommend not to default to any specific version, because it's very > likely that someone will end up bumping the "Version:" header in > brief.el without realizing that it needs to be updated elsewhere. > > Some with all the places where you say "5.86" in the README: better > reword it such that it doesn't need to be updated when the version > number changes. > >> +# Search for brief.el or brief.elc through default path list > > Why not just assume that the `brief` package was properly installed, > and hence `emacs -l brief` will just work™? > >> +#!/bin/bash > > Do you actually make use of bash-isms? If so, can we avoid them > without too much extra work? If not, we should say "/bin/sh". > >> +exec ${EMACS} --load ${BRIEFPATH}/brief --eval \ >> +"(progn \ >> + (setq-default truncate-lines t) \ >> + (setq scroll-step 1 \ >> + scroll-conservatively 101) \ >> + (setq hscroll-margin 1 \ >> + hscroll-step 1) \ >> + (scroll-bar-mode -1) \ >> + (brief-mode 1))" \ >> +"${EMACSARGS[@]}" > > Why are these (h)scroll settings here instead of adding them to > brief-mode? > > If you want to keep `brief-mode` as a mode which doesn't impose > particular (h)scroll settings (which is probably a good idea, indeed), > then you could add a new function to brief.el and call that function > instead of `brief-mode`. > >> diff --git a/packages/brief/b.el b/packages/brief/b.el >> new file mode 100644 >> index 0000000..4a0ab6b >> --- /dev/null >> +++ b/packages/brief/b.el >> @@ -0,0 +1,9 @@ >> +(setq-default truncate-lines t) >> +;;(setq-default global-visual-line-mode t) >> +(setq scroll-step 1 >> + scroll-conservatively 101) >> +(setq hscroll-margin 1 >> + hscroll-step 1) >> +(scroll-bar-mode -1) >> +(load "~/bin/elisp/brief/brief") >> +(brief-mode 1) > > Several problems here: > - I don't see any place where this file is used. > - the chance that (load "~/bin/elisp/brief/brief") will work > on the user's system is fairly slim. > - this file will be in the `load-path` so (load "b") will load it and so > will (require 'b). I think we'd rather keep such short names for more > popular packages (like the `s` package). > - this Elisp file doesn't follow the convention that loading a file > doesn't have significant effects (i.e. it just defines new vars and > functions). > >> diff --git a/packages/brief/brief.el b/packages/brief/brief.el >> index 1dd3181..66d3790 100644 >> --- a/packages/brief/brief.el >> +++ b/packages/brief/brief.el >> @@ -1,11 +1,11 @@ >> -;;; brief.el --- Brief Editor Emulator >> +;;; brief.el --- Brief Editor Emulator (Brief Mode) > > The Commentary says > > ;; On Linux: (including native X11 Emacs, terminal mode Emacs on xterm > ;; and Linux X11 Emacs running on VcXsrv (1.19.3.4+) under > ;; Win10): > ;; Emacs 23.3.1, 24.4.50.2, 25.2.2, 26.0.50, 26.1 and 27.0.50. > > has 5.86 still been tested with all those Emacs versions? > Any chance you'd be willing to drop compatibility with Emacs-23 (which > doesn't come with package.el)? > >> +(defcustom brief-search-replace-using-regexp t >> + "An option determine if search & replace using regular expression or string. >> +This is a buffer local variable with default value 't which means >> +regular expression is used for search & replace commands by default." >> + :type 'boolean >> + :group 'brief) > > The `:group 'brief` is redundant (it defaults to the last group defined > in the same file). Same applies to all other defcustoms. > Oh, and the docstring doesn't need to say "An option" ;-) > >> (defcustom brief-fake-region-face 'region ; 'secondary-selection >> "The face (color) of fake region when `brief-search-fake-region-mark' is t." >> - :type 'boolean >> + :type 'sexp >> :group 'brief) > > You probably want `:type 'face` here. > >> +(defvar-local brief-search-overlay nil >> + "A buffer local overlay which records the current search selection. >> +It is deleted when the search ends or region deactivated.") > > It should probably use a "--" in the name. > >> (defvar brief-latest-killed-buffer-info nil >> - "This variable records the info of the most recently killed file >> -buffer. If user accidentally killed a file buffer, it can be >> -recovered accordingly. >> + "This variable records the info of the most recently killed file buffer. >> +If user accidentally killed a file buffer, it can be recovered >> +accordingly. >> Information is a list of: > > As above, the docstring doesn't need to say "This variable records". > >> +(defmacro brief-meta-l-key (updown key) >> + "Define key function and associated the key in `brief-prefix-meta-l'." >> + (let* ((dir (symbol-name updown)) >> + (keydesc (key-description key)) >> + (keyfunc (intern (concat "brief-mark-line-" dir "-with-" keydesc)))) >> + `(progn >> + (defun ,keyfunc () >> + ,(concat "Mark line " dir " with " keydesc " key") >> + (interactive) >> + (,(intern (concat "brief-call-mark-line-" dir "-with-key")) >> + ,key)) >> + (define-key brief-prefix-meta-l ,key ',keyfunc)))) > > This macro name and docstring makes it sound like it works for anything > one might want to have under the M-l prefix, yet the code seems to > limit this to those few operations named > brief-call-mark-line-*-with-key, so it's much more focused than announced. > >> (call-interactively >> '(lambda (filename) > > Please don't quote your lambdas. > >> + (let* (c1 >> + (p1 (save-excursion >> + (end-of-visual-line) >> + (setq c1 (following-char)) (point))) >> + (p2 (save-excursion >> + (end-of-line) (point)))) ;; `end-of-line' of course is at crlf > > I think you can turn this into: > > (let* ((p1 (save-excursion > (end-of-visual-line) > (point))) > (c1 (char-after p1)) > (p2 (line-end-position))) ;; `end-of-line' of course is at crlf > >> +(defun brief-toggle-auto-backup () >> + "Toggle auto-backup on or off." >> + (interactive) >> + (message "Turn Emacs auto-backup %s" >> + (if (setq auto-save-default (not auto-save-default)) >> + "ON" "OFF"))) > > You could use a minor mode, here: > > (define-minor-mode brief-auto-backup-mode > "Whether auto-backup is done" > :global t > :variable auto-save-default) > >> +(defun brief-beginning-of-file () >> + "Goto beginning of file." >> + (interactive "^") >> + (goto-char (point-min))) > > Why not (defalias 'brief-beginning-of-file #'beginning-of-buffer) ? > >> +(defun brief-end-of-file () >> + "Goto end of file." >> + (interactive "^") >> + (goto-char (point-max))) > > Why not (defalias 'brief-end-of-file #'end-of-buffer) ? > >> +(defun brief-open-new-line-next () >> + "Open a new line right below the current line and go there." >> + (interactive) >> + (move-end-of-line 1) >> + (newline)) > > Why not (defalias 'brief-open-new-line-next #'open-line) ? > > > Stefan