[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [elpa] master 74818d5: Brief Mode v5.86 release.
From: |
Stefan Monnier |
Subject: |
Re: [elpa] master 74818d5: Brief Mode v5.86 release. |
Date: |
Thu, 18 Oct 2018 11:43:47 -0400 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux) |
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))" \
> +"address@hidden"
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
- Re: [elpa] master 74818d5: Brief Mode v5.86 release.,
Stefan Monnier <=