unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#53153: package-quickstart: unusual autoload form in selectrum gives byte-compilation warning
@ 2022-01-10  4:46 Stefan Kangas
  2022-01-13  9:11 ` Lars Ingebrigtsen
  2022-01-13 14:46 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 2 replies; 6+ messages in thread
From: Stefan Kangas @ 2022-01-10  4:46 UTC (permalink / raw)
  To: 53153; +Cc: Stefan Monnier

Severity: minor

I am seeing this warning whenever I install a package:

  Compiling file /home/skangas/.emacs.d/package-quickstart.el at Mon
Jan 10 05:33:47 2022
  package-quickstart.el:2060:2170: Warning: defcustom for ‘selectrum-mode’ fails
      to specify containing group

This is due to an odd hack in selectrum.el (available on MELPA or at
https://github.com/raxod502/selectrum):

  ;; You may ask why we copy the entire minor-mode definition into the
  ;; autoloads file, and autoload several private functions as well.
  ;; This is because enabling `selectrum-mode' does not actually require
  ;; any of the code in Selectrum. So, to improve startup time, we avoid
  ;; loading Selectrum when enabling `selectrum-mode'.

  ;;;###autoload
  (progn
    (define-minor-mode selectrum-mode

This unusual arrangement means that we end up with this interesting form
in package-quickstart.el (pretty-printed):

  (define-minor-mode selectrum-mode
    "Minor mode to use Selectrum for `completing-read'."
    :global t
    (if selectrum-mode (progn
                         (selectrum-mode -1)
                         (setq selectrum-mode t)
                         (setq selectrum--old-completing-read-function
(default-value 'completing-read-function))
                         (setq-default completing-read-function
#'selectrum-completing-read)
                         (setq selectrum--old-read-buffer-function
(default-value 'read-buffer-function))
                         (setq-default read-buffer-function
#'selectrum-read-buffer)
                         (setq selectrum--old-read-file-name-function
(default-value 'read-file-name-function))
                         (setq-default read-file-name-function
#'selectrum-read-file-name)
                         (setq
selectrum--old-completion-in-region-function (default-value

      'completion-in-region-function))
                         (when selectrum-complete-in-buffer
                           (setq-default completion-in-region-function
#'selectrum-completion-in-region))
                         (advice-add #'completing-read-multiple
:override #'selectrum-completing-read-multiple)
                         (advice-add 'dired-read-dir-and-switches :around

#'selectrum--fix-dired-read-dir-and-switches)
                         (advice-add 'read-library-name :override
#'selectrum-read-library-name)
                         (advice-add #'minibuffer-message :around
#'selectrum--fix-minibuffer-message)
                         (define-key minibuffer-local-map [remap
previous-matching-history-element]
                                     'selectrum-select-from-history))
      (when (equal (default-value 'completing-read-function)
#'selectrum-completing-read)
        (setq-default completing-read-function
selectrum--old-completing-read-function))
      (when (equal (default-value 'read-buffer-function)
#'selectrum-read-buffer)
        (setq-default read-buffer-function selectrum--old-read-buffer-function))
      (when (equal (default-value 'read-file-name-function)
#'selectrum-read-file-name)
        (setq-default read-file-name-function
selectrum--old-read-file-name-function))
      (when (equal (default-value 'completion-in-region-function)
#'selectrum-completion-in-region)
        (setq-default completion-in-region-function
selectrum--old-completion-in-region-function))
      (advice-remove #'completing-read-multiple
#'selectrum-completing-read-multiple)
      (advice-remove 'dired-read-dir-and-switches
#'selectrum--fix-dired-read-dir-and-switches)
      (advice-remove 'read-library-name #'selectrum-read-library-name)
      (advice-remove #'minibuffer-message #'selectrum--fix-minibuffer-message)
      (when (eq (lookup-key minibuffer-local-map [remap
previous-matching-history-element])
                #'selectrum-select-from-history)
        (define-key minibuffer-local-map [remap
previous-matching-history-element] nil))))

Now, I have two questions:

1. Does it make sense for Emacs to be complaining about a defcustom
   here?  AFAICT, there is no defcustom in the above form.  Do we care,
   or is this use case just unsupported?

2. I guess the other question is why `selectrum-mode' is not just in its
   own file, if startup speed is of the essence.  Maybe Stefan Monnier
   has something to add here.





^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#53153: package-quickstart: unusual autoload form in selectrum gives byte-compilation warning
  2022-01-10  4:46 bug#53153: package-quickstart: unusual autoload form in selectrum gives byte-compilation warning Stefan Kangas
@ 2022-01-13  9:11 ` Lars Ingebrigtsen
  2022-01-13 11:20   ` Stefan Kangas
  2022-01-13 14:46 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 1 reply; 6+ messages in thread
From: Lars Ingebrigtsen @ 2022-01-13  9:11 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: 53153, Stefan Monnier

Stefan Kangas <stefan@marxist.se> writes:

> This unusual arrangement means that we end up with this interesting form
> in package-quickstart.el (pretty-printed):
>
>   (define-minor-mode selectrum-mode
>     "Minor mode to use Selectrum for `completing-read'."
>     :global t

[...]

> 1. Does it make sense for Emacs to be complaining about a defcustom
>    here?  AFAICT, there is no defcustom in the above form.  Do we care,
>    or is this use case just unsupported?

define-minor-mode expands to (among many other things) a defcustom form,
I think?

> 2. I guess the other question is why `selectrum-mode' is not just in its
>    own file, if startup speed is of the essence.  Maybe Stefan Monnier
>    has something to add here.

Yeah, it seems like a pretty weird thing to do.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#53153: package-quickstart: unusual autoload form in selectrum gives byte-compilation warning
  2022-01-13  9:11 ` Lars Ingebrigtsen
@ 2022-01-13 11:20   ` Stefan Kangas
  0 siblings, 0 replies; 6+ messages in thread
From: Stefan Kangas @ 2022-01-13 11:20 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 53153, Stefan Monnier

Lars Ingebrigtsen <larsi@gnus.org> writes:

> define-minor-mode expands to (among many other things) a defcustom form,
> I think?

Aha, yes.  So here's a minimal reproducer:

  testel="$(mktemp).el"
  echo '(define-minor-mode foo-mode "" :global t)' > $testel
  emacs -Q --batch --eval "(byte-compile-file \"$testel\")"

This gives:

  In toplevel form:
  tmp.mmmKTQXZHb.el:1:1: Warning: defcustom for ‘foo-mode’ fails to
specify containing group
  tmp.mmmKTQXZHb.el:1:1: Warning: defcustom for ‘foo-mode’ fails to
specify containing group





^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#53153: package-quickstart: unusual autoload form in selectrum gives byte-compilation warning
  2022-01-10  4:46 bug#53153: package-quickstart: unusual autoload form in selectrum gives byte-compilation warning Stefan Kangas
  2022-01-13  9:11 ` Lars Ingebrigtsen
@ 2022-01-13 14:46 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2022-01-13 15:30   ` Stefan Kangas
  2022-01-13 16:20   ` bug#53153: [External] : " Drew Adams
  1 sibling, 2 replies; 6+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-01-13 14:46 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: 53153

> This unusual arrangement means that we end up with this interesting form
> in package-quickstart.el (pretty-printed):
>
>   (define-minor-mode selectrum-mode
>     "Minor mode to use Selectrum for `completing-read'."
>     :global t
>     (if selectrum-mode (progn
[...]

Global minor modes define a `defcustom` and its :group is usually
provided by default by the last `defgroup` but if you copy this
`define-minor-mode` to some other file (such as an autoloads file), then
you can't rely on this defaulting so you should provide
a `:group` explicitly.

Regarding using such a "trick" or placing the minor mode in its own
file, both approaches have their advantages and disadvantages so while
it's unusual I don't see a problem with it.


        Stefan






^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#53153: package-quickstart: unusual autoload form in selectrum gives byte-compilation warning
  2022-01-13 14:46 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2022-01-13 15:30   ` Stefan Kangas
  2022-01-13 16:20   ` bug#53153: [External] : " Drew Adams
  1 sibling, 0 replies; 6+ messages in thread
From: Stefan Kangas @ 2022-01-13 15:30 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 53153

tags 53153 + notabug
close 53153
thanks

> Global minor modes define a `defcustom` and its :group is usually
> provided by default by the last `defgroup` but if you copy this
> `define-minor-mode` to some other file (such as an autoloads file), then
> you can't rely on this defaulting so you should provide
> a `:group` explicitly.

Right, so this is notabug and I'm tagging it as such and closing.
I have opened this PR against selectrum instead:

    https://github.com/raxod502/selectrum/pull/584

> Regarding using such a "trick" or placing the minor mode in its own
> file, both approaches have their advantages and disadvantages so while
> it's unusual I don't see a problem with it.

That's a good point.





^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#53153: [External] : bug#53153: package-quickstart: unusual autoload form in selectrum gives byte-compilation warning
  2022-01-13 14:46 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2022-01-13 15:30   ` Stefan Kangas
@ 2022-01-13 16:20   ` Drew Adams
  1 sibling, 0 replies; 6+ messages in thread
From: Drew Adams @ 2022-01-13 16:20 UTC (permalink / raw)
  To: Stefan Monnier, Stefan Kangas; +Cc: 53153

> Global minor modes define a `defcustom` and its
> :group is usually provided by default by the last 
> `defgroup` but if you copy this `define-minor-mode`
> to some other file (such as an autoloads file),
> then you can't rely on this defaulting so you
> should provide a `:group` explicitly.

FWIW -

IMO, it's misguided to recommend that people not
use :group explicitly when they can get away
with depending on the preceding :group being for
the same group.

That's a sort of "premature visual optimization"
that provides no real advantage and presents the
disadvantages of (1) being less clear (at the
limit, it requires readers of the code to search
backward) and (2) gotchas such as the one you
described.

People no doubt have different feelings about
such things, which is fine - it's partly an
individual style choice.  I disagree with
recommending one or the other as a general
guideline.

But if one of them is to be used as a guideline
it should be to specify :group explicitly, even
when it might not be strictly necessary.  Clear
for all.

And what's the purported advantage of omitting
:group when it's not needed?  Presumably it's
 (1) less clutter/noise and perhaps (2) easier
to notice when the "current" :group changes.

#1 is very minor, at best.  :group labels are
not verbose.

#2 can be misleading.  If a reader of the code
tries to depend on each :group being significant
(i.e., it can't be omitted), that assumption
will bite, sooner or later.

(Just one opinion.)





^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-01-13 16:20 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-10  4:46 bug#53153: package-quickstart: unusual autoload form in selectrum gives byte-compilation warning Stefan Kangas
2022-01-13  9:11 ` Lars Ingebrigtsen
2022-01-13 11:20   ` Stefan Kangas
2022-01-13 14:46 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-01-13 15:30   ` Stefan Kangas
2022-01-13 16:20   ` bug#53153: [External] : " Drew Adams

Code repositories for project(s) associated with this 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).