From: Stefan Monnier <monnier@iro.umontreal.ca>
To: emacs-devel@gnu.org
Subject: Re: [elpa] master 74818d5: Brief Mode v5.86 release.
Date: Fri, 19 Oct 2018 11:35:29 -0400 [thread overview]
Message-ID: <jwv7eiet0fi.fsf-monnier+gmane.emacs.devel@gnu.org> (raw)
In-Reply-To: jwvzhvbwbhb.fsf-monnier+emacsdiffs@gnu.org
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 <monnier@IRO.UMontreal.CA> 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
next prev parent reply other threads:[~2018-10-19 15:35 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20181018141131.22680.53035@vcs0.savannah.gnu.org>
[not found] ` <20181018141132.6E8BA208EC@vcs0.savannah.gnu.org>
2018-10-18 15:43 ` [elpa] master 74818d5: Brief Mode v5.86 release Stefan Monnier
2018-10-19 15:35 ` Stefan Monnier [this message]
2018-10-19 15:41 ` Stefan Monnier
2018-10-19 15:47 ` 路客
2018-10-19 16:47 ` Stefan Monnier
2018-10-20 16:01 ` 路客
2018-10-20 18:51 ` Stefan Monnier
2018-10-21 3:40 ` 路客
2018-10-21 14:23 ` Stefan Monnier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=jwv7eiet0fi.fsf-monnier+gmane.emacs.devel@gnu.org \
--to=monnier@iro.umontreal.ca \
--cc=emacs-devel@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).