Can we not introduce frivolous indentation change to define-minor-mode?

Can we not introduce frivolous indentation change to define-minor-mode?

[Leo Liu]

Hi there,

Just noticed this indentation change (only recently started using emacs
25) and it's annoying. Why the change?

Leo

Re: Can we not introduce frivolous indentation change to define-minor-mode?

[Eli Zaretskii]

> From: Leo Liu <sdl.web@gmail.com>
> Date: Tue, 05 Jan 2016 20:32:10 +0800
> 
> Just noticed this indentation change (only recently started using emacs
> 25) and it's annoying. Why the change?

Please tell the details, I don't quite understand what change do you
refer to.

Thanks.

Re: Can we not introduce frivolous indentation change to define-minor-mode?

[Leo Liu]

On 2016-01-05 18:09 +0200, Eli Zaretskii wrote:
> Please tell the details, I don't quite understand what change do you
> refer to.
>
> Thanks.

The introduction of (indent 1) to define-minor-mode. I have quite a few
define-minor-mode instances starting like this:

(define-minor-mode some_fancy_mode nil
  ...)

The indentation is changed between emacs < 25 and 25.

Leo

Re: Can we not introduce frivolous indentation change to define-minor-mode?

[Oleh Krehel]

Hi Leo,

Leo Liu <sdl.web@gmail.com> writes:

> The introduction of (indent 1) to define-minor-mode. I have quite a few
> define-minor-mode instances starting like this:
>
> (define-minor-mode some_fancy_mode nil
>   ...)
>
> The indentation is changed between emacs < 25 and 25.

I added an (indent 1) statement around a year ago. The reason is that it
didn't have one before, and that (indent 1) is obviously the correct
indentation level.

Every `define-minor-mode' statement in the core (and org-mode, and most
of ELPA) already obeys the (intent 1) convention. It seems that you were
defining minor modes in your config (and ELPA) with nil instead of the
docstring. I suggest to either add a docstring or put a newline after
the minor mode name.

Oleh

Re: Can we not introduce frivolous indentation change to define-minor-mode?

[Leo Liu]

Hi Oleh,

On 2016-01-06 10:14 +0100, Oleh Krehel wrote:
> I added an (indent 1) statement around a year ago. The reason is that it
> didn't have one before, and that (indent 1) is obviously the correct
> indentation level.
>
> Every `define-minor-mode' statement in the core (and org-mode, and most
> of ELPA) already obeys the (intent 1) convention. It seems that you were
> defining minor modes in your config (and ELPA) with nil instead of the
> docstring. I suggest to either add a docstring or put a newline after
> the minor mode name.
>
> Oleh

The doc-string generated by define-minor-mode is fine in many cases so
`nil' is legitimate. Secondly those forms already indent correctly
without introducing (indent 1). What (indent 1) does is penalise people
like me whose years-old code now indents differently. If there are no
good reasons I'd like to revert the change to keep the behaviour
consistent with previous emacsen. WDYT?

Leo

Re: Can we not introduce frivolous indentation change to define-minor-mode?

[Oleh Krehel]

Leo Liu <sdl.web@gmail.com> writes:

> Secondly those forms already indent correctly without introducing
> (indent 1)

Not true. Here's the indentation without an indent spec:

(define-minor-mode ace-window-display-mode
    "Minor mode for showing the ace window key in the mode line."
  :global t)

Here's the correct indentation with (indent 1) spec:

(define-minor-mode ace-window-display-mode
  "Minor mode for showing the ace window key in the mode line."
  :global t)

This applies when (setq lisp-indent-function 'common-lisp-indent-function)
is used.

> I'd like to revert the change to keep the behaviour
> consistent with previous emacsen. WDYT?

I'm against the revert. I'd like to keep using
`common-lisp-indent-function' without extra configuration.

Re: Can we not introduce frivolous indentation change to define-minor-mode?

[Artur Malabarba]

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

On Jan 6, 2016 10:38 AM, "Oleh Krehel" <ohwoeowho@gmail.com> wrote:
>
> Leo Liu <sdl.web@gmail.com> writes:
>
> > Secondly those forms already indent correctly without introducing
> > (indent 1)
>
> Not true. Here's the indentation without an indent spec:
>
> (define-minor-mode ace-window-display-mode
>     "Minor mode for showing the ace window key in the mode line."
>   :global t)
>
> Here's the correct indentation with (indent 1) spec:
>
> (define-minor-mode ace-window-display-mode
>   "Minor mode for showing the ace window key in the mode line."
>   :global t)
>
> This applies when (setq lisp-indent-function 'common-lisp-indent-function)
> is used.

What if you set the indent spec to defun? Does that work?

[-- Attachment #2: Type: text/html, Size: 1067 bytes --]

Re: Can we not introduce frivolous indentation change to define-minor-mode?

[Oleh Krehel]

Artur Malabarba <bruce.connor.am@gmail.com> writes:

> What if you set the indent spec to defun? Does that work?

What do you mean here? The indent spec of `defun' is (indent 2), it's
not justified for `define-minor-mode', since then it would have to look
like:

(define-minor-mode ace-window-display-mode "Minor mode for showing the ace window key in the mode line."
  :global t)

Re: Can we not introduce frivolous indentation change to define-minor-mode?

[Artur Malabarba]

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

On Jan 6, 2016 11:56 AM, "Oleh Krehel" <ohwoeowho@gmail.com> wrote:
>
> What do you mean here? The indent spec of `defun' is (indent 2),

Ah, never mind that then. My mistake.

[-- Attachment #2: Type: text/html, Size: 290 bytes --]

Re: Can we not introduce frivolous indentation change to define-minor-mode?

[Jonas Bernoulli]

Oleh Krehel <ohwoeowho@gmail.com> writes:

> Artur Malabarba <bruce.connor.am@gmail.com> writes:
>
>> What if you set the indent spec to defun? Does that work?
>
> What do you mean here? The indent spec of `defun' is (indent 2), it's
> not justified for `define-minor-mode',

The `defun' indent variant should be used, not the indent variant used
by `defun'.  (I assume the `defun' indent value was originally used by
`defun' and got its name that way.  But this is (no longer) the case,0
which makes this a bit confusing.)

With

  (defmacro define-minor-mode (...)
    "..."
    (declare ... (indent defun))
    ...)

we get

  (define-minor-mode foo-mode
    "doc-string"
    :global t)

and

  (define-minor-mode foo-mode nil
    :global t)

So everyone can be happy again.

Re: Can we not introduce frivolous indentation change to define-minor-mode?

[Oleh Krehel]

Jonas Bernoulli <jonas@bernoul.li> writes:

> With
>
>   (defmacro define-minor-mode (...)
>     "..."
>     (declare ... (indent defun))
>     ...)
>
> we get
>
>   (define-minor-mode foo-mode
>     "doc-string"
>     :global t)

Thanks for explaining, but that's not what I get with my config:

    (setq lisp-indent-function 'common-lisp-indent-function)

It works fine only for the default setting:

    (setq lisp-indent-function 'lisp-indent-function)

which is problematic for me in several other areas (indentation of
quoted lists, vectors, cl-labels etc).

Re: Can we not introduce frivolous indentation change to define-minor-mode?

[Dmitry Gutov]

On 01/06/2016 05:48 PM, Oleh Krehel wrote:

> It works fine only for the default setting:
>
>      (setq lisp-indent-function 'lisp-indent-function)

We use the default setting in the Emacs sources, and recommend it for 
third-party Elisp as well. If you're adamant on using 
common-lisp-indent-function, why not change the `indent' property of 
`define-minor-mode' in your own config?

> which is problematic for me in several other areas (indentation of
> quoted lists, vectors, cl-labels etc).

As discussed in the relevant bug, we'll gladly take a patch for quoted 
lists. For vectors too, I assume; I haven't noticed this problem much 
before.

Re: Can we not introduce frivolous indentation change to define-minor-mode?

[Oleh Krehel]

Dmitry Gutov <dgutov@yandex.ru> writes:

> We use the default setting in the Emacs sources, and recommend it for
> third-party Elisp as well. If you're adamant on using
> common-lisp-indent-function, why not change the `indent' property of
> `define-minor-mode' in your own config?

I assumed the change would be beneficial to all users of
`common-lisp-indent-function' and irrelevant to all users of
`lisp-indent-function'. And having a more precise metadata never hurts
as well.

Re: Can we not introduce frivolous indentation change to define-minor-mode?

[Leo Liu]

Hi Oleh,

On 2016-01-06 17:02 +0100, Oleh Krehel wrote:
> I assumed the change would be beneficial to all users of
> `common-lisp-indent-function' and irrelevant to all users of
> `lisp-indent-function'. And having a more precise metadata never hurts
> as well.

All changes are meant well so no worries.

On the matter at hand I have reverted the change. If you insist on it
please discuss with Emacs devels. For now staying consistent with older
emacsen weighs more.

Cheers,
Leo

Re: Can we not introduce frivolous indentation change to define-minor-mode?

[Eli Zaretskii]

> From: Leo Liu <sdl.web@gmail.com>
> Date: Thu, 07 Jan 2016 09:54:09 +0800
> Cc: Jonas Bernoulli <jonas@bernoul.li>, emacs-devel <emacs-devel@gnu.org>,
> 	Artur Malabarba <bruce.connor.am@gmail.com>,
> 	Dmitry Gutov <dgutov@yandex.ru>
> 
> On the matter at hand I have reverted the change. If you insist on it
> please discuss with Emacs devels. For now staying consistent with older
> emacsen weighs more.

I'm sorry, but the practice of unilaterally reverting commits of
others seems unkind to me.  I think we should refrain from making
changes which are under dispute, where the parties have not yet
reached an agreement.  Doing so causes aggravation and animosity we'd
better without.

John, WDYT?

Re: Can we not introduce frivolous indentation change to define-minor-mode?

[John Wiegley]

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

>>>>> Eli Zaretskii <eliz@gnu.org> writes:

> I'm sorry, but the practice of unilaterally reverting commits of others
> seems unkind to me. I think we should refrain from making changes which are
> under dispute, where the parties have not yet reached an agreement. Doing so
> causes aggravation and animosity we'd better without.

> John, WDYT?

100% agreement.

-- 
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: Can we not introduce frivolous indentation change to define-minor-mode?

[Leo Liu]

On 2016-01-07 09:58 -0800, John Wiegley wrote:
>> I'm sorry, but the practice of unilaterally reverting commits of others
>> seems unkind to me. I think we should refrain from making changes which are
>> under dispute, where the parties have not yet reached an agreement. Doing so
>> causes aggravation and animosity we'd better without.
>
>> John, WDYT?
>
> 100% agreement.

No disagreement here.

I don't intend to appear unkind or anything like that. I hope my message
doesn't suggest that.

I have weighted on the comments provided and taken the action
accordingly based on the fact that it is an incompatible change and
controversial on a few points.

Cheers,
Leo

Re: Can we not introduce frivolous indentation change to define-minor-mode?

[John Wiegley]

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

>>>>> Leo Liu <sdl.web@gmail.com> writes:

> I have weighted on the comments provided and taken the action accordingly
> based on the fact that it is an incompatible change and controversial on a
> few points.

Our main concern here is establishing a policy for the future that avoids the
potential for negative reaction in these cases.

On this particular commit, what you did might even be appreciated by the
original author; but hearing them agree first would have been better.

-- 
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: Can we not introduce frivolous indentation change to define-minor-mode?

[Leo Liu]

On 2016-01-07 11:00 -0800, John Wiegley wrote:
> Our main concern here is establishing a policy for the future that avoids the
> potential for negative reaction in these cases.
>
> On this particular commit, what you did might even be appreciated by the
> original author; but hearing them agree first would have been better.

Indeed John.

I think I jumped the gun a bit too quick though the consensus appears
(to me) imminent. My faults and time constraints often lead me to prefer
quickly move things out of the way which is something I'll try my best
to avoid.

To Oleh,

Sorry for this accident. Not meant to be unkind/rude.

Leo

Re: Can we not introduce frivolous indentation change to define-minor-mode?

[Eli Zaretskii]

> From: Leo Liu <sdl.web@gmail.com>
> Date: Fri, 08 Jan 2016 02:49:34 +0800
> 
> >> I'm sorry, but the practice of unilaterally reverting commits of others
> >> seems unkind to me. I think we should refrain from making changes which are
> >> under dispute, where the parties have not yet reached an agreement. Doing so
> >> causes aggravation and animosity we'd better without.
> >
> >> John, WDYT?
> >
> > 100% agreement.
> 
> No disagreement here.
> 
> I don't intend to appear unkind or anything like that. I hope my message
> doesn't suggest that.
> 
> I have weighted on the comments provided and taken the action
> accordingly based on the fact that it is an incompatible change and
> controversial on a few points.

I'm sure you have considered the issue before reverting.

However, my point is that you should have refrained from that until
the discussion concluded in an agreement.  If no agreement could be
reached, and no acceptable compromise was in sight, the proper way,
IMO, would be to ask John to suggest a compromise or make a decision.
This way, all the involved parties will at least have an opportunity
to present their case, and will feel that due process has been done.

Thanks.