GNU bug report logs - #32496
27.0.50; Strange indentation when ruby-align-chained-calls is t

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 32496 in the body.
You can then email your comments to 32496 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.50; Strange indentation when ruby-align-chained-calls is t
Date: Wed, 22 Aug 2018 08:36:06 -0300

[Message part 1 (text/plain, inline)]

1. (setq ruby-align-chained-calls t)
2. (setq ruby-use-smie t)
3. Open a file in ruby-mode, insert the following and indent it

----------
some_variable.where.not(x: nil)
             .where(y: 2)
----------

Expected behaviour: Nothing would happen, the code is already properly
indented.

What actually happens: The code gets indented as follows

----------
some_variable.where.not(x: nil)
                   .where(y: 2)
----------

Note that this is conflicts with the indentation enforced by rubocop.
Artur

[Message part 2 (text/html, inline)]

Message #8 received at 32496 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bruce.connor.am <at> gmail.com, 32496 <at> debbugs.gnu.org
Subject: Re: bug#32496: 27.0.50; Strange indentation when
 ruby-align-chained-calls is t
Date: Wed, 22 Aug 2018 15:50:05 +0300

On 8/22/18 2:36 PM, Artur Malabarba wrote:
> 1. (setq ruby-align-chained-calls t)
> 2. (setq ruby-use-smie t)
> 3. Open a file in ruby-mode, insert the following and indent it
> 
> ----------
> some_variable.where.not(x: nil)
>               .where(y: 2)
> ----------
> 
> Expected behaviour: Nothing would happen, the code is already properly
> indented.
> 
> What actually happens: The code gets indented as follows
> 
> ----------
> some_variable.where.not(x: nil)
>                     .where(y: 2)
> ----------
> 
> Note that this is conflicts with the indentation enforced by rubocop.

I'd like to point out that this is exactly the behavior Bozhidar asked 
for, back when this variable was introduced. See:

http://lists.gnu.org/archive/html/emacs-devel/2014-01/msg01802.html

and in particular the Example 1 in the referenced comment:

https://github.com/rubocop-hq/ruby-style-guide/pull/176#issuecomment-18664622

So we even have a test (ruby-align-chained-calls) that check that the 
alignment is do to the last dot, and not to the first one.

Message #11 received at 32496 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 32496 <at> debbugs.gnu.org, Bozhidar Batsov <bozhidar <at> batsov.com>
Subject: Re: bug#32496: 27.0.50;
 Strange indentation when ruby-align-chained-calls is t
Date: Sat, 27 Oct 2018 19:22:16 -0300

[Message part 1 (text/plain, inline)]

IIUC, bozhidar was requesting that the dots be aligned to the dot above (as
opposed to being indented by only 2 spaces). He didn't say what should
happen if one of the lines has multiple dots in it.

The linked github comment does explicitly request the aligning to the last
dot, but it's the only comment that requests that on a very long discussion
that was largely focused on a different topic (whether or not to use
trailing dots).

Bozhidar, do you have an opinion on this?

Artur


On Wed, 22 Aug 2018 at 09:50, Dmitry Gutov <dgutov <at> yandex.ru> wrote:

> On 8/22/18 2:36 PM, Artur Malabarba wrote:
> > 1. (setq ruby-align-chained-calls t)
> > 2. (setq ruby-use-smie t)
> > 3. Open a file in ruby-mode, insert the following and indent it
> >
> > ----------
> > some_variable.where.not(x: nil)
> >               .where(y: 2)
> > ----------
> >
> > Expected behaviour: Nothing would happen, the code is already properly
> > indented.
> >
> > What actually happens: The code gets indented as follows
> >
> > ----------
> > some_variable.where.not(x: nil)
> >                     .where(y: 2)
> > ----------
> >
> > Note that this is conflicts with the indentation enforced by rubocop.
>
> I'd like to point out that this is exactly the behavior Bozhidar asked
> for, back when this variable was introduced. See:
>
> http://lists.gnu.org/archive/html/emacs-devel/2014-01/msg01802.html
>
> and in particular the Example 1 in the referenced comment:
>
>
> https://github.com/rubocop-hq/ruby-style-guide/pull/176#issuecomment-18664622
>
> So we even have a test (ruby-align-chained-calls) that check that the
> alignment is do to the last dot, and not to the first one.
>

[Message part 2 (text/html, inline)]

Message #14 received at 32496 <at> debbugs.gnu.org (full text, mbox):

From: Bozhidar Batsov <bozhidar <at> batsov.com>
To: Artur Malabarba <bruce.connor.am <at> gmail.com>
Cc: 32496 <at> debbugs.gnu.org, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#32496: 27.0.50;
 Strange indentation when ruby-align-chained-calls is t
Date: Sun, 18 Nov 2018 09:36:17 +0100

[Message part 1 (text/plain, inline)]

Sorry for the radio silence - I've been super busy lately.

It's hard for me to understand the indentation in the examples in the email
(as it seems the same to me). Very simply put - the idea is to align
multi-line chained calls on the `.`, as opposed to just nest them under the
root receiver as we'd normally do.

I think Dmitry implemented this great and it's behaving just as it's
supposed to be behaving. Perhaps you misunderstood how this was supposed to
behave? What's the indentation you expected?

On Sun, 28 Oct 2018 at 00:22, Artur Malabarba <bruce.connor.am <at> gmail.com>
wrote:

> IIUC, bozhidar was requesting that the dots be aligned to the dot above
> (as opposed to being indented by only 2 spaces). He didn't say what should
> happen if one of the lines has multiple dots in it.
>
> The linked github comment does explicitly request the aligning to the last
> dot, but it's the only comment that requests that on a very long discussion
> that was largely focused on a different topic (whether or not to use
> trailing dots).
>
> Bozhidar, do you have an opinion on this?
>
> Artur
>
>
> On Wed, 22 Aug 2018 at 09:50, Dmitry Gutov <dgutov <at> yandex.ru> wrote:
>
>> On 8/22/18 2:36 PM, Artur Malabarba wrote:
>> > 1. (setq ruby-align-chained-calls t)
>> > 2. (setq ruby-use-smie t)
>> > 3. Open a file in ruby-mode, insert the following and indent it
>> >
>> > ----------
>> > some_variable.where.not(x: nil)
>> >               .where(y: 2)
>> > ----------
>> >
>> > Expected behaviour: Nothing would happen, the code is already properly
>> > indented.
>> >
>> > What actually happens: The code gets indented as follows
>> >
>> > ----------
>> > some_variable.where.not(x: nil)
>> >                     .where(y: 2)
>> > ----------
>> >
>> > Note that this is conflicts with the indentation enforced by rubocop.
>>
>> I'd like to point out that this is exactly the behavior Bozhidar asked
>> for, back when this variable was introduced. See:
>>
>> http://lists.gnu.org/archive/html/emacs-devel/2014-01/msg01802.html
>>
>> and in particular the Example 1 in the referenced comment:
>>
>>
>> https://github.com/rubocop-hq/ruby-style-guide/pull/176#issuecomment-18664622
>>
>> So we even have a test (ruby-align-chained-calls) that check that the
>> alignment is do to the last dot, and not to the first one.
>>
>

[Message part 2 (text/html, inline)]

Message #17 received at 32496 <at> debbugs.gnu.org (full text, mbox):

From: Wendel Scardua <wendel.scardua <at> voltera.com.br>
To: 32496 <at> debbugs.gnu.org
Subject: Re: bug#32496: 27.0.50;
 Strange indentation when ruby-align-chained-calls is t
Date: Fri, 11 Sep 2020 14:16:52 -0300

[Message part 1 (text/plain, inline)]

I was about to open a bug about this same issue. Seeing this thread, there
seems to have been some misunderstanding between the parts.

Bhozidar thought it was about indenting on the dot vs indenting according
to the previous line, and in that aspect the feature is working correctly.

But when there are multiple dots on a line, their style guide enforces
indenting on the *first* dot, not the last, while (setq
ruby-align-chained-calls t) - supposedly based on it - enforces the latter.

[Message part 2 (text/html, inline)]

Message #20 received at 32496 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Bozhidar Batsov <bozhidar <at> batsov.com>
Cc: 32496 <at> debbugs.gnu.org, Artur Malabarba <bruce.connor.am <at> gmail.com>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#32496: 27.0.50; Strange indentation when
 ruby-align-chained-calls is t
Date: Wed, 01 Sep 2021 11:53:16 +0200

Bozhidar Batsov <bozhidar <at> batsov.com> writes:

> Sorry for the radio silence - I've been super busy lately. 
>
> It's hard for me to understand the indentation in the examples in the email
> (as it seems the same to me). Very simply put - the idea is to align multi-line
> chained calls on the `.`, as opposed to just nest them under the root receiver
> as we'd normally do.
>
> I think Dmitry implemented this great and it's behaving just as it's supposed
> to be behaving. Perhaps you misunderstood how this was supposed to
> behave? What's the indentation you expected? 

(I'm going through old bug reports that unfortunately weren't resolved
at the time.)

The examples were in HTML mail, so they were difficult to understand.

Emacs (with (setq ruby-align-chained-calls t)) currently aligns like
this:

some_variable.where
             .not(x: nil)
             .where(y: 2)

Which is correct.  However, when there's a mixture of keeping things on
one line and breaking, we get this:

some_variable.where.not(x: nil)
                   .where(y: 2)

I think the bug reporter wants:

some_variable.where.not(x: nil)
             .where(y: 2)

I.e., align multiline chained calls on the first dot, not the last?

(I don't know Ruby, so I have no opinion here.)

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

Message #23 received at 32496 <at> debbugs.gnu.org (full text, mbox):

From: "Bozhidar Batsov" <bozhidar <at> batsov.dev>
To: "Lars Ingebrigtsen" <larsi <at> gnus.org>
Cc: 32496 <at> debbugs.gnu.org, Artur Malabarba <bruce.connor.am <at> gmail.com>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#32496: 27.0.50;
  Strange indentation when ruby-align-chained-calls is t
Date: Wed, 01 Sep 2021 13:02:56 +0300

[Message part 1 (text/plain, inline)]

Ah, I finally understood the issue at hand! It's really hard to discuss indentation problems in e-mail. :D

Yeah, I can confirm there's a bug when using (setq ruby-align-chained-calls t) in this example:

some_variable.where.not(x: nil)
                         .where(y: 2)

The two `where`s should be lined up, but currently the second `where` is lined up with the `not`.

some_variable.where.not(x: nil)
                                    .where(y: 2)

I can also confirm that the first behavior is what RuboCop (Ruby's popular linter/formatter) expects, when configured for aligned chained method calls. 

On Wed, Sep 1, 2021, at 12:53 PM, Lars Ingebrigtsen wrote:
> Bozhidar Batsov <bozhidar <at> batsov.com> writes:
> 
> > Sorry for the radio silence - I've been super busy lately. 
> >
> > It's hard for me to understand the indentation in the examples in the email
> > (as it seems the same to me). Very simply put - the idea is to align multi-line
> > chained calls on the `.`, as opposed to just nest them under the root receiver
> > as we'd normally do.
> >
> > I think Dmitry implemented this great and it's behaving just as it's supposed
> > to be behaving. Perhaps you misunderstood how this was supposed to
> > behave? What's the indentation you expected? 
> 
> (I'm going through old bug reports that unfortunately weren't resolved
> at the time.)
> 
> The examples were in HTML mail, so they were difficult to understand.
> 
> Emacs (with (setq ruby-align-chained-calls t)) currently aligns like
> this:
> 
> some_variable.where
>              .not(x: nil)
>              .where(y: 2)
> 
> Which is correct.  However, when there's a mixture of keeping things on
> one line and breaking, we get this:
> 
> some_variable.where.not(x: nil)
>                    .where(y: 2)
> 
> I think the bug reporter wants:
> 
> some_variable.where.not(x: nil)
>              .where(y: 2)
> 
> I.e., align multiline chained calls on the first dot, not the last?
> 
> (I don't know Ruby, so I have no opinion here.)
> 
> -- 
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no
> 

[Message part 2 (text/html, inline)]

Message #26 received at 32496 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: "Bozhidar Batsov" <bozhidar <at> batsov.dev>
Cc: 32496 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Artur Malabarba <bruce.connor.am <at> gmail.com>, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#32496: 27.0.50; Strange indentation when
 ruby-align-chained-calls is t
Date: Thu, 02 Sep 2021 08:55:50 +0200

"Bozhidar Batsov" <bozhidar <at> batsov.dev> writes:

> Ah, I finally understood the issue at hand! It's really hard to discuss
> indentation problems in e-mail. :D
>
> Yeah, I can confirm there's a bug when using (setq ruby-align-chained-calls t)
> in this example:
>
> some_variable.where.not(x: nil)
>                    .where(y: 2)
>
> The two `where`s should be lined up, but currently the second `where` is
> lined up with the `not`.

So this is coming from:

    ('(:before . ".")
     (if (smie-rule-sibling-p)
         (and ruby-align-chained-calls 0)
       (smie-backward-sexp ".")
       (cons 'column (+ (current-column)
                        ruby-indent-level))))

In the aligned case, I think we should back up to the first "." in the
chain and use that as the column?  But my SMIE-fu is pretty much
non-existent, so I've added Stefan to the CCs.

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

Message #29 received at 32496 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 32496 <at> debbugs.gnu.org, Bozhidar Batsov <bozhidar <at> batsov.dev>,
 Artur Malabarba <bruce.connor.am <at> gmail.com>, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#32496: 27.0.50; Strange indentation when
 ruby-align-chained-calls is t
Date: Wed, 08 Sep 2021 15:01:08 -0400

Lars Ingebrigtsen [2021-09-02 08:55:50] wrote:

> "Bozhidar Batsov" <bozhidar <at> batsov.dev> writes:
>
>> Ah, I finally understood the issue at hand! It's really hard to discuss
>> indentation problems in e-mail. :D
>>
>> Yeah, I can confirm there's a bug when using (setq ruby-align-chained-calls t)
>> in this example:
>>
>> some_variable.where.not(x: nil)
>>                    .where(y: 2)
>>
>> The two `where`s should be lined up, but currently the second `where` is
>> lined up with the `not`.
>
> So this is coming from:
>
>     ('(:before . ".")
>      (if (smie-rule-sibling-p)
>          (and ruby-align-chained-calls 0)
>        (smie-backward-sexp ".")
>        (cons 'column (+ (current-column)
>                         ruby-indent-level))))
>
> In the aligned case, I think we should back up to the first "." in the
> chain and use that as the column?  But my SMIE-fu is pretty much
> non-existent, so I've added Stefan to the CCs.

You could try something like

    diff --git a/lisp/progmodes/ruby-mode.el b/lisp/progmodes/ruby-mode.el
    index c09f007a5ee..c681800f6a7 100644
    --- a/lisp/progmodes/ruby-mode.el
    +++ b/lisp/progmodes/ruby-mode.el
    @@ -640,7 +640,15 @@ ruby-smie-rules
         ('(:before . "do") (ruby-smie--indent-to-stmt))
         ('(:before . ".")
          (if (smie-rule-sibling-p)
    -         (and ruby-align-chained-calls 0)
    +         (when ruby-align-chained-calls
    +           (while
    +               (let ((pos (point))
    +                     (parent (smie-backward-sexp ".")))
    +                 (if (not (equal (nth 2 parent) "."))
    +                     (progn (goto-char pos) nil)
    +                   (goto-char (nth 1 parent))
    +                   (not (smie-rule-bolp)))))
    +           (cons 'column (current-column)))
            (smie-backward-sexp ".")
            (cons 'column (+ (current-column)
                             ruby-indent-level))))
    @@ -826,13 +834,6 @@ ruby--electric-indent-p
     
     ;; FIXME: Remove this?  It's unused here, but some redefinitions of
     ;; `ruby-calculate-indent' in user init files still call it.
    -(defun ruby-current-indentation ()
    -  "Return the indentation level of current line."
    -  (save-excursion
    -    (beginning-of-line)
    -    (back-to-indentation)
    -    (current-column)))
    -
     (defun ruby-indent-line (&optional ignored)
       "Correct the indentation of the current Ruby line."
       (interactive)

But please add corresponding regression tests,


        Stefan

Message #32 received at 32496 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 32496 <at> debbugs.gnu.org, Bozhidar Batsov <bozhidar <at> batsov.dev>,
 Artur Malabarba <bruce.connor.am <at> gmail.com>, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#32496: 27.0.50; Strange indentation when
 ruby-align-chained-calls is t
Date: Thu, 09 Sep 2021 16:25:46 +0200

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

> You could try something like

[...]

> But please add corresponding regression tests,

Thanks; seems to work perfectly.  (And I've added some more tests, but
this one was already being tested, I found out afterwards.)

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.