Compilation warnings in mouse.el

Compilation warnings in mouse.el

[Eli Zaretskii]

Could you please fix these?

  mouse.el:541:1:Warning: defcustom for `mouse-select-region-move-to-beginning'
      fails to specify containing group
  mouse.el:541:1:Warning: defcustom for `mouse-select-region-move-to-beginning'
      fails to specify containing group

TIA

Re: Compilation warnings in mouse.el

[Stephen Berman]

On Mon, 11 Jul 2016 17:23:47 +0300 Eli Zaretskii <eliz@gnu.org> wrote:

> Could you please fix these?
>
>   mouse.el:541:1:Warning: defcustom for `mouse-select-region-move-to-beginning'
>       fails to specify containing group
>   mouse.el:541:1:Warning: defcustom for `mouse-select-region-move-to-beginning'
>       fails to specify containing group

Sorry about that.  I saw that the other defcustoms in mouse.el have a
":group 'mouse" line and wrongly assumed this group was defined.  So
should I just add ":group 'mouse" to that defcustom or instead add a
defgroup to mouse.el and remove the existing ":group 'mouse" lines?

Steve Berman

Re: Compilation warnings in mouse.el

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Cc: emacs-devel@gnu.org
> Date: Mon, 11 Jul 2016 23:55:55 +0200
> 
> >   mouse.el:541:1:Warning: defcustom for `mouse-select-region-move-to-beginning'
> >       fails to specify containing group
> >   mouse.el:541:1:Warning: defcustom for `mouse-select-region-move-to-beginning'
> >       fails to specify containing group
> 
> Sorry about that.  I saw that the other defcustoms in mouse.el have a
> ":group 'mouse" line and wrongly assumed this group was defined.  So
> should I just add ":group 'mouse" to that defcustom or instead add a
> defgroup to mouse.el and remove the existing ":group 'mouse" lines?

Unlike Stefan, I think we should add :group to every defcustom.
Having them mysteriously missing from some of them is a time bomb:
remove or add enough defcustoms in the same file, and you have a bug.

Are there any downsides to having :group in all defcustoms?

Re: Compilation warnings in mouse.el

[Stephen Berman]

On Tue, 12 Jul 2016 08:04:57 +0300 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Stephen Berman <stephen.berman@gmx.net>
>> Cc: emacs-devel@gnu.org
>> Date: Mon, 11 Jul 2016 23:55:55 +0200
>> 
>> >   mouse.el:541:1:Warning: defcustom for `mouse-select-region-move-to-beginning'
>> >       fails to specify containing group
>> >   mouse.el:541:1:Warning: defcustom for `mouse-select-region-move-to-beginning'
>> >       fails to specify containing group
>> 
>> Sorry about that.  I saw that the other defcustoms in mouse.el have a
>> ":group 'mouse" line and wrongly assumed this group was defined.  So
>> should I just add ":group 'mouse" to that defcustom or instead add a
>> defgroup to mouse.el and remove the existing ":group 'mouse" lines?
>
> Unlike Stefan, I think we should add :group to every defcustom.
> Having them mysteriously missing from some of them is a time bomb:
> remove or add enough defcustoms in the same file, and you have a bug.

I added the :group line in commit 2f67f8a.

Steve Berman

Re: Compilation warnings in mouse.el

[John Wiegley]

[-- Attachment #1: Type: text/plain, Size: 454 bytes --]

>>>>> "EZ" == Eli Zaretskii <eliz@gnu.org> writes:

EZ> Unlike Stefan, I think we should add :group to every defcustom. Having
EZ> them mysteriously missing from some of them is a time bomb: remove or add
EZ> enough defcustoms in the same file, and you have a bug.

I'm with Eli on this one.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 629 bytes --]

Re: Compilation warnings in mouse.el

[Stefan Monnier]

EZ> Unlike Stefan, I think we should add :group to every defcustom. Having
EZ> them mysteriously missing from some of them is a time bomb: 

The way I see it, defcustoms should pretty much never have :group, and
the group to which they belong is simply determined by the file in which
they occur.

E.g. the `mouse' group should be defined in mouse.el rather than in
cus-edit.el, which would have solved this problem.

EZ> remove or add
EZ> enough defcustoms in the same file, and you have a bug.

I don't see how removing/adding defcustoms in the same file
would introduce problems.


        Stefan


PS: Incidentally, we have two definitions of the `mouse' group
in cus-edit.el:

    % grep -A2 '(defgroup mouse ' **/*.el
    lisp/cus-edit.el:(defgroup mouse nil
    lisp/cus-edit.el-  "Mouse support."
    lisp/cus-edit.el-  :group 'editing)
    --
    lisp/cus-edit.el:(defgroup mouse nil
    lisp/cus-edit.el-  "Input from the mouse."
    lisp/cus-edit.el-  :group 'environment)
    %

Re: Compilation warnings in mouse.el

[Joost Kremers]

On Tue, Jul 12 2016, Stefan Monnier wrote:
> EZ> Unlike Stefan, I think we should add :group to every defcustom. Having
> EZ> them mysteriously missing from some of them is a time bomb: 
>
> The way I see it, defcustoms should pretty much never have :group, and
> the group to which they belong is simply determined by the file in which
> they occur.

That would mean that in multifile packages the defcustoms must always be
put in one single file, which must be named according to the
customisation group one wants to create. And creating multiple
customisaton groups for a single package (not uncommon in larger
packages) would require multiple files.

That's to say, such a system would require that the code of a package be
organised along the lines of the customisation groups it provides. But
customisation is a UI feature, and as such does not necessarily present
the best way to organise the code.


-- 
Joost Kremers
Life has its moments

Re: Compilation warnings in mouse.el

[Stefan Monnier]

EZ> Unlike Stefan, I think we should add :group to every defcustom. Having
EZ> them mysteriously missing from some of them is a time bomb: 
>> The way I see it, defcustoms should pretty much never have :group, and
>> the group to which they belong is simply determined by the file in which
>> they occur.
> That would mean that in multifile packages the defcustoms must always be
> put in one single file,

Notice I said "pretty much".  Obviously, there will be exceptions, so
we do want to keep the :group for those situations.

> which must be named according to the customisation group one wants to
> create.  And creating multiple customisaton groups for a single package
> (not uncommon in larger packages) would require multiple files.

The current system (default to the last group defined in the current
file) lets you define several groups along with their defcustoms in
a single file without the use of :group.

> That's to say, such a system would require that the code of a package be
> organised along the lines of the customisation groups it provides.  But
> customisation is a UI feature, and as such does not necessarily present
> the best way to organise the code.

Agreed.  But in practice the most common situation is to have the
defgroup in the same file as the corresponding defcustoms.

Maybe a bit less so in multi-file packages (especially less so in
*small* multi-file packages, I think).


        Stefan

RE: Compilation warnings in mouse.el

[Drew Adams]

What is the problem that _not_ requiring or warning about
a missing :group tries to solve?

Stefan: Are you encouraging this only in order to allow
laziness and slightly less verbose code, or is there an
actual problem that occurs if people consistently provide
:group with defcustoms?

"Defcustoms should pretty much never have :group" is pretty
strong ("_should_"), even if you do qualify it with "pretty
much".  Why _should_ they?  Is it just to be less rigid, or
is there really a _problem_ that this solves?

It's pretty strong when someone as knowledgeable and
authoritative in Emacs as yourself says that something
_should_ be some way.  It helps if you say why.

Re: Compilation warnings in mouse.el

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Tue, 12 Jul 2016 18:58:12 -0400
> 
> EZ> Unlike Stefan, I think we should add :group to every defcustom. Having
> EZ> them mysteriously missing from some of them is a time bomb: 
> 
> The way I see it, defcustoms should pretty much never have :group, and
> the group to which they belong is simply determined by the file in which
> they occur.

But as long as such a system isn't installed, we shouldn't behave as
if it were.  (And what you propose is not without downsides, I think.)

> EZ> remove or add
> EZ> enough defcustoms in the same file, and you have a bug.
> 
> I don't see how removing/adding defcustoms in the same file
> would introduce problems.

We just saw such a problem, no?

> PS: Incidentally, we have two definitions of the `mouse' group
> in cus-edit.el:

Thanks.  Bonus points for fixing that.

Re: Compilation warnings in mouse.el

[Stefan Monnier]

>> The way I see it, defcustoms should pretty much never have :group, and
>> the group to which they belong is simply determined by the file in which
>> they occur.
> But as long as such a system isn't installed, we shouldn't behave as
> if it were.  (And what you propose is not without downsides, I think.)

Such a system has been installed ever since

    commit d3b80e9b70eaa0edb4cfc0d91543c41929fa70c0
    Author: Stefan Monnier <monnier@iro.umontreal.ca>
    Date:   Sun Nov 18 01:35:12 2001 +0000

        (custom-current-group-alist): New var.
        (custom-declare-group): Set it.
        (custom-current-group): New fun.
        (custom-declare-variable, custom-handle-all-keywords):
        Use it as a default if no :group argument is specified.

>> I don't see how removing/adding defcustoms in the same file
>> would introduce problems.
> We just saw such a problem, no?

I must have missed something.  All I saw was that someone added
a defcustom in mouse.el and did not put a :group while there is no
defgroup in that file (and all other defcustoms in there have a :group).

That seems pretty far from "removing/adding defcustoms in the same file".

>> PS: Incidentally, we have two definitions of the `mouse' group
>> in cus-edit.el:
> Thanks.  Bonus points for fixing that.

Hmm... Git didn't tell me how many points I got!

BTW, I'm not really opposed to the use of :group in general.

But I'm opposed to having it be mandatory in the obvious case of
a single-file single-group package, where the :group args are just
redundant.  My above 2001 commit was designed to solve that case and
it's proved to work fine since.


        Stefan

RE: Compilation warnings in mouse.el

[Drew Adams]

> BTW, I'm not really opposed to the use of :group in general.
> 
> But I'm opposed to having it be mandatory in the obvious case of
> a single-file single-group package, where the :group args are just
> redundant.

Why?  That's the question that has not been answered?  Is it just
to allow laziness and less verbose code, or is there some real
problem that it intends to solve?

> My above 2001 commit was designed to solve that case and
> it's proved to work fine since.

"Solve?"  What's the _problem_ with expecting :group when a
group is intended?

Re: Compilation warnings in mouse.el

[Stefan Monnier]

> Why?  That's the question that has not been answered?

I'd expect any programmer to know that redundancy is generally a problem.
It drowns the important information, and introduces bugs when the
redundant copies aren't automatically maintained in-sync.

You call it laziness, I call it software engineering.


        Stefan

RE: Compilation warnings in mouse.el

[Drew Adams]

> > Why?  That's the question that has not been answered?
> 
> I'd expect any programmer to know

Oh, please.

> that redundancy is generally a problem.
> It drowns the important information, and introduces bugs when the
> redundant copies aren't automatically maintained in-sync.
> 
> You call it laziness, I call it software engineering.

So this aims to reduce redundancy.  Thanks for making that clear.

But a :group specifies, within a given defcustom, that the option
belongs to a given group.  It is not redundant.  And it is "important
information."

It's a local declaration - the option's group membership is an
attribute of the option definition (defcustom), encapsulated as
part of that definition.

An alternative, if you are worried about such local declaration,
could be to declare all of the members of a group within the group
definition.  (That would be worse, of course.)

What you in effect prefer is implicit zones of a file that
correspond to a given group (up to the next defgroup).

If you have only one file per group and one group per file, the
file becomes, in effect, a module for its defined options and
faces.  The defgroup says that everything belongs to that group.

And if you have multiple files per group then what?  Does the 
first use of :group have the same effect for the files in the
group that do not have a defgroup?  Or do you redundantly add
the same defgroup to each such file?

And if you have multiple groups per file then each such "module"
is a zone of the file from one defgroup to the next.  (Or perhaps
to the next :group for a different group?)

This is hardly simpler, easier to read, and less error-prone, IMO.

I'd vote for the requiring at least one :group per option/face
definition.

By "require" I really mean a byte-compiler warning.  I don't mind
the implicit :group you favor, as long as users also get a warning
whenever there is no :group (not just when there is none and there
is no preceding defgroup).

Let them then sort out the complexity described above, when they
get such a warning.  The warning, and Emacs in general, should
encourage the use of :group - at least one per option/face, IMO.

You may call such local declaration "redundancy", if you like.

Just because you _can_ come up with a system that can save some
:group declarations (at the cost of added complexity for users
and increased error-proneness), that does not mean that's a good
idea.  Such a shortcut doesn't save much and reduces clarity.
KIS.

Re: Compilation warnings in mouse.el

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Wed, 13 Jul 2016 11:12:35 -0400
> 
> >> The way I see it, defcustoms should pretty much never have :group, and
> >> the group to which they belong is simply determined by the file in which
> >> they occur.
> > But as long as such a system isn't installed, we shouldn't behave as
> > if it were.  (And what you propose is not without downsides, I think.)
> 
> Such a system has been installed ever since
> 
>     commit d3b80e9b70eaa0edb4cfc0d91543c41929fa70c0
>     Author: Stefan Monnier <monnier@iro.umontreal.ca>
>     Date:   Sun Nov 18 01:35:12 2001 +0000
> 
>         (custom-current-group-alist): New var.
>         (custom-declare-group): Set it.
>         (custom-current-group): New fun.
>         (custom-declare-variable, custom-handle-all-keywords):
>         Use it as a default if no :group argument is specified.

No, I meant a system where one doesn't have to do anything to have a
group for a defcustom.  Nothing at all.

If they must first check if there's a defgroup in the same file, and
then if there's more than one defgroup, make sure the defcustom is
after the right one, then this is an error-prone system which cannot
be trusted.

> >> I don't see how removing/adding defcustoms in the same file
> >> would introduce problems.
> > We just saw such a problem, no?
> 
> I must have missed something.  All I saw was that someone added
> a defcustom in mouse.el and did not put a :group while there is no
> defgroup in that file (and all other defcustoms in there have a :group).
> 
> That seems pretty far from "removing/adding defcustoms in the same file".

OK, s/defcustom/defgroup/.

> But I'm opposed to having it be mandatory in the obvious case of
> a single-file single-group package, where the :group args are just
> redundant.  My above 2001 commit was designed to solve that case and
> it's proved to work fine since.

IME, solving part of a problem doesn't really solve it.

Re: Compilation warnings in mouse.el

[Stefan Monnier]

> No, I meant a system where one doesn't have to do anything to have a
> group for a defcustom.  Nothing at all.

I guess we could do that, but I think we'd hear even more screams.

> If they must first check if there's a defgroup in the same file, and

The byte-compiler will complain if a defcustom doesn't have a :group and
there's no preceding defgroup (as was the case in the situation that
started this discussion).  So I don't think it's too much of a problem.

> then if there's more than one defgroup, make sure the defcustom is
> after the right one, then this is an error-prone system which cannot
> be trusted.

Then let's restrict the current system so it also complains if there's
no :group yet there are more than one preceding defgroup.


        Stefan

RE: Compilation warnings in mouse.el

[Drew Adams]

> Then let's restrict the current system so it also complains if there's
> no :group yet there are more than one preceding defgroup.

Getting more complex, not less.  More stuff for the user to figure
out - just what's wrong, what's the rule, what do I need to do in
this particular (multi-group) context.

Re: Compilation warnings in mouse.el

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Wed, 13 Jul 2016 11:42:45 -0400
> 
> > If they must first check if there's a defgroup in the same file, and
> 
> The byte-compiler will complain if a defcustom doesn't have a :group and
> there's no preceding defgroup (as was the case in the situation that
> started this discussion).  So I don't think it's too much of a problem.

It's evidently a problem.  Otherwise, I wouldn't have had to ask
Stephen to take care of those warnings.

> > then if there's more than one defgroup, make sure the defcustom is
> > after the right one, then this is an error-prone system which cannot
> > be trusted.
> 
> Then let's restrict the current system so it also complains if there's
> no :group yet there are more than one preceding defgroup.

I think this cure will be worse than the decease.

Re: Compilation warnings in mouse.el

[Stefan Monnier]

>> > If they must first check if there's a defgroup in the same file, and
>> The byte-compiler will complain if a defcustom doesn't have a :group and
>> there's no preceding defgroup (as was the case in the situation that
>> started this discussion).  So I don't think it's too much of a problem.
> It's evidently a problem.  Otherwise, I wouldn't have had to ask
> Stephen to take care of those warnings.

As long as Git doesn't grow an AI extension which automatically guesses
that a ":group 'mouse" was meant here, I don't see how we could have
a better solution.


        Stefan

Re: Compilation warnings in mouse.el

[Dmitry Gutov]

On 07/12/2016 08:04 AM, Eli Zaretskii wrote:

> Unlike Stefan, I think we should add :group to every defcustom.

The additions to each file just have to be consistent with what's 
already there. The presently discussed patch, wasn't.

Speaking of personal preference, however, I'd rather using the `mouse' 
group didn't work, in the absence of a defgroup somewhere.

> Having them mysteriously missing from some of them is a time bomb:
> remove or add enough defcustoms in the same file, and you have a bug.

How would removing a defcustom, or merely adding one, lead to a bug?

The new defcustom might have :group unspecified, and if there's no 
defgroup in the current file, it would become a problem, but that's 
unrelated to any defcustom's already present in the file.

Re: Compilation warnings in mouse.el

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 13 Jul 2016 01:56:30 +0300
> 
> On 07/12/2016 08:04 AM, Eli Zaretskii wrote:
> 
> > Unlike Stefan, I think we should add :group to every defcustom.
> 
> The additions to each file just have to be consistent with what's 
> already there. The presently discussed patch, wasn't.

But if each defcustom has its :group, then the need for consistency is
no longer a requirement, is it?

> Speaking of personal preference, however, I'd rather using the `mouse' 
> group didn't work, in the absence of a defgroup somewhere.

If we can do something to that effect, I don't think I will object.

> > Having them mysteriously missing from some of them is a time bomb:
> > remove or add enough defcustoms in the same file, and you have a bug.
> 
> How would removing a defcustom, or merely adding one, lead to a bug?

Well, not bug, a compilation warning that suddenly appears out of
nowhere for code that was there for a long time.

> The new defcustom might have :group unspecified, and if there's no 
> defgroup in the current file, it would become a problem, but that's 
> unrelated to any defcustom's already present in the file.

AFAIU, extant defcustoms could start causing warnings, given some
changes in other defcustom's.  Am I mistaken?

In short, I think having one defcustom depend on another is bad for
maintenance.

Re: Compilation warnings in mouse.el

[Dmitry Gutov]

On 07/13/2016 05:30 PM, Eli Zaretskii wrote:

> But if each defcustom has its :group, then the need for consistency is
> no longer a requirement, is it?

The need for consistency is always there when one is writing code.

>> Speaking of personal preference, however, I'd rather using the `mouse'
>> group didn't work, in the absence of a defgroup somewhere.
>
> If we can do something to that effect, I don't think I will object.

We could start with a warning in Emacs 26, and make it so in 28-ish, maybe.

>> How would removing a defcustom, or merely adding one, lead to a bug?
>
> Well, not bug, a compilation warning that suddenly appears out of
> nowhere for code that was there for a long time.

For new code, AFAIU. Not for code that has been there for a while.

> AFAIU, extant defcustoms could start causing warnings, given some
> changes in other defcustom's.  Am I mistaken?

I believe so.

> In short, I think having one defcustom depend on another is bad for
> maintenance.

It would be bad, if it were so. defcustom depend on defgroup.

RE: Compilation warnings in mouse.el

[Drew Adams]

> Unlike Stefan, I think we should add :group to every defcustom.
> Having them mysteriously missing from some of them is a time bomb:
> remove or add enough defcustoms in the same file, and you have a bug.

100% agreement.  I don't even think we should have introduced this
"shortcut" or "timesaver" feature.  It serves no purpose and can
lead to problems.

> Are there any downsides to having :group in all defcustoms?

Not that I can see.  I'd like to see it be required, but if it's
too late for that then I'd like to see a compiler warning at least.