byte-compile-nogroup-warn effectively disabled

byte-compile-nogroup-warn effectively disabled

[Richard M Stallman]

This change

     ;; Warn if a custom definition fails to specify :group.
     (defun byte-compile-nogroup-warn (form)

    +  (if (and (memq (car form) '(custom-declare-face custom-declare-variable))
    +           byte-compile-current-group)
    +      ;; The group will be provided implicitly.
    +      nil

effectively defeats the point of that warning, which is to
make sure that every defcustom and defface specifies the group.

Re: byte-compile-nogroup-warn effectively disabled

[Stefan Monnier]

> This change
>      ;; Warn if a custom definition fails to specify :group.
>      (defun byte-compile-nogroup-warn (form)

>     +  (if (and (memq (car form) '(custom-declare-face custom-declare-variable))
>     +           byte-compile-current-group)
>     +      ;; The group will be provided implicitly.
>     +      nil

> effectively defeats the point of that warning, which is to
> make sure that every defcustom and defface specifies the group.

I know it's your opinion, and I disagree.  Even without an
explicit :group, the defcustom will have a proper group attributed to it
because it will use the last group defined in the file.  So, in my
opinion it fixes the warning so that it only occurs in cases where the
lack of the :group really leads to the variable being associated to no
group at all.


        Stefan

RE: byte-compile-nogroup-warn effectively disabled

[Drew Adams]

> > effectively defeats the point of that warning, which is to
> > make sure that every defcustom and defface specifies the group.
> 
> I know it's your opinion, and I disagree.  Even without an
> explicit :group, the defcustom will have a proper group 
> attributed to it
> because it will use the last group defined in the file.  So, in my
> opinion it fixes the warning so that it only occurs in cases where the
> lack of the :group really leads to the variable being associated to no
> group at all.

Sorry, I haven't followed this. But IIUYC, I think it would be a mistake to
automatically assign an option or face the group that was last specified before
it in the same file.

What for? What is gained? Sounds like an invitation for user errors that won't
necessarily be noticed right away. It might not be an important error to assign
the wrong group, but why encourage that possibility?

What do you do when more than one :group is used in the previous defcustom? Do
you use the last one in that defcustom? Until now, :group order within a
defcustom has had no importance. Or do you use all of the :groups of that
defcustom?

In Icicles, for instance, I have several groups of options - for minibuffer
highlighting, completion behavior, *Completions* display, buffer treatment, key
bindings, key completion, matching, and so on. In some csaes, an option belongs
to more than one such group. I generally present the options in alphabetic order
in the file. There is no reason at all to assume that the next option in the
file should have the same group(s) as the previous one. If I forget to add a
:group, I would rather see a warning and have no group assigned than to have
some inappropriate group (not "a proper group") assigned. 

I don't see the point of this. Personally, I don't care much one way or the
other about the warnings - they are just an extra aid. But automatically
assigning groups sounds like asking for (minor) trouble.

RE: byte-compile-nogroup-warn effectively disabled

[Stephen J. Turnbull]

Drew Adams writes:

 > What for? What is gained? Sounds like an invitation for user errors
 > that won't necessarily be noticed right away. It might not be an
 > important error to assign the wrong group, but why encourage that
 > possibility?

I often use this style:

(defgroup a)
(defcustom a-1)
(defcustom a-2)

(defgroup b)
(defcustom b-1)
(defcustom b-2)

I think that's clean and natural, and I think it's pointless to
require a :group in every defcustom when that is the style being used.
Furthermore, in most cases I only use one group in a file; a module
large enough to need two custom groups is usually large enough to be
two modules.

Since a large minority of my defcustoms are at most 5 lines long with
the :group line, that's often a 25% increase in the number of
defcustoms I can get on the screen.

 > There is no reason at all to assume that the next option in the
 > file should have the same group(s) as the previous one.

That's not what Stefan said, he said it gets the last group defined
(which I assume means defgroup'ed) in the file.

RE: byte-compile-nogroup-warn effectively disabled

[Drew Adams]

>  > There is no reason at all to assume that the next option in the
>  > file should have the same group(s) as the previous one.
> 
> That's not what Stefan said, he said it gets the last group defined
> (which I assume means defgroup'ed) in the file.

Then I misunderstood that part; sorry. But it's irrelevant to the points I
raised. However the group is chosen from among those groups defined in the file,
there is no reason to assume that it is the appropriate one for defcustoms that
have no :group.

(I assume nothing happens if no group is defined in the file, even if there are
some :groups used in some defcustoms.)

Wrt to the rest of what you write, it seems you are arguing that it might be
convenient for some people who follow certain coding practices, since it would
mean they could skip the burden of adding :group to each definition, with the
side benefit of less verbose code.

That doesn't sound convincing, to me. This convenience for some in some use
cases will open the door to errors by all.

The one case where it might be relatively benign is if there is only one group
definition in the file, which you say is your common use case anyway.

Re: byte-compile-nogroup-warn effectively disabled

[Richard M Stallman]

    Sorry, I haven't followed this. But IIUYC, I think it would be a mistake to
    automatically assign an option or face the group that was last specified before
    it in the same file.

    What for? What is gained? Sounds like an invitation for user errors that won't
    necessarily be noticed right away. It might not be an important error to assign
    the wrong group, but why encourage that possibility?

That is what I feel, too.  Omitting the :group is no big savings in
effort; writing it gives you a chance to think about it.

RE: byte-compile-nogroup-warn effectively disabled

[Stephen J. Turnbull]

Drew Adams writes:

 > Then I misunderstood that part; sorry. But it's irrelevant to the
 > points I raised. However the group is chosen from among those
 > groups defined in the file, there is no reason to assume that it is
 > the appropriate one for defcustoms that have no :group.

Please don't be unnecessarily contentious.  I just gave you two: at
least one natural style has that property, and most files will have
only one defgroup.  (After this I will refer to "the 'natural' style"
in quotes, to remind the reader that it's not the only reasonable
style.)

 > (I assume nothing happens if no group is defined in the file, even
 > if there are some :groups used in some defcustoms.)

In that case with Stefan's code a warning is issued.  I'm not sure if
that qualifies as "nothing"?

 > Wrt to the rest of what you write, it seems you are arguing that it
 > might be convenient for some people who follow certain coding
 > practices, since it would mean they could skip the burden of adding
 > :group to each definition, with the side benefit of less verbose
 > code.

I'm arguing that it eliminates redundancy, is less verbose, and
encourages appropriate modularity.  The "burden of adding :group" is
not an issue, as that can easily be automated.

In fact, except for the fact that a lot of my tools currently don't
handle def-forms as I would want when they're not at toplevel, I'd be
perfectly happy with an extended defgroup syntax:

(defgroup a           ; docstring, parent group, etc go here
  (defcustom a-1)
  (defface a-2)
)

and a warning for def-forms without :group at toplevel.  That may not
be very Lisp-y, though.

 > That doesn't sound convincing, to me. This convenience for some in
 > some use cases will open the door to errors by all.

"Opening the door to errors" is FUD.  The question is how error-prone
is it?  I would argue that your practice of explicitly selecting the
group(s) for each object at the time of writing the defface or
defcustom is far more error-prone in maintenance, and at least
somewhat error-prone at write-time.

Should you refactor the groups, you'll have to collect them all and
examine each member to be sure that it ends up in the right group.
Using the "natural" organization, you have all the relevant forms in
one place, separated by defgroups that make the current rationale
clear.  When writing the def-forms, you have to keep an accurate model
of the group structure in your head.  But what if you go back and
refactor halfway?  Will you remember all of the relevant def-forms to
change?  Again, in this case under the "natural" organization all will
be in one place, and most will be in the same few groups.

RE: byte-compile-nogroup-warn effectively disabled

[Drew Adams]

>  > However the group is chosen from among those groups
>  > defined in the file, there is no reason to assume that it
>  > is the appropriate one for defcustoms that have no :group.
> ...
> I just gave you two: at least one natural style has that property,
> and most files will have only one defgroup.

I already agreed that it would be OK if restricted to that subset (I said, "if
there is only one group definition in the file").

That is not the general case, however. You've described and argued to support a
particular case where you find it useful and where I agreed there is no great
problem. Why not restrict the proposed behavior to that case? Especially if
"most files will have only one defgroup", that should satisfy you.

> In fact, except for the fact that a lot of my tools currently don't
> handle def-forms as I would want when they're not at toplevel, I'd be
> perfectly happy with an extended defgroup syntax:
> 
> (defgroup a   ; docstring, parent group, etc go here
>   (defcustom a-1)
>   (defface a-2))

That's a horse of a different color. I have no problem with adding something
like that.

I would also have no problem with a form that doesn't define a new group but
just declares the group for multiple options and faces:

(custom-group a   ; or some other function name
  (defcustom a-1)
  (defface a-2))

Or even allowing multiple groups:

(custom-group (convenience hypermedia)
  (defcustom a-1)
  (defface a-2))

> your practice of explicitly selecting the
> group(s) for each object at the time of writing the defface or
> defcustom is far more error-prone in maintenance, and at least
> somewhat error-prone at write-time.

Different strokes for different folks. There is not only one use case or only
one right or reasonable approach to coding and code maintenance.

Re: byte-compile-nogroup-warn effectively disabled

[Stefan Monnier]

>     Sorry, I haven't followed this. But IIUYC, I think it would be
>     a mistake to automatically assign an option or face the group that
>     was last specified before it in the same file.

>     What for? What is gained? Sounds like an invitation for user
>     errors that won't necessarily be noticed right away. It might not
>     be an important error to assign the wrong group, but why encourage
>     that possibility?

> That is what I feel, too.  Omitting the :group is no big savings in
> effort; writing it gives you a chance to think about it.

I know.  And we've already reached the conclusion that we have to agree
to disagree on this one.


        Stefan