GNU bug report logs - #16593
24.3.50; ruby-mode: align chained method calls on multiple lines

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 16593 in the body.
You can then email your comments to 16593 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: Dmitry Gutov <dgutov <at> yandex.ru>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.3.50; ruby-mode: align chained method calls on multiple lines
Date: Thu, 30 Jan 2014 06:24:50 +0200

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

Tags: patch

For background:
http://lists.gnu.org/archive/html/emacs-devel/2014-01/msg01889.html

I have a working patch, but I can't get rid of the warnings on startup:

Warning (smie): Conflict: . </= .
Warning (smie): Conflict: . </= .

[ruby-chained-calls.diff (text/x-diff, inline)]

=== modified file 'lisp/progmodes/ruby-mode.el'
--- lisp/progmodes/ruby-mode.el	2014-01-17 03:15:02 +0000
+++ lisp/progmodes/ruby-mode.el	2014-01-30 04:18:54 +0000
@@ -264,6 +264,13 @@
   :safe 'listp
   :version "24.4")
 
+(defcustom ruby-align-chained-calls nil
+  "If non-nil, chained method calls on multiple lines will be
+aligned to the same column."
+  :type 'boolean
+  :group 'ruby
+  :safe 'booleanp)
+
 (defcustom ruby-deep-arglist t
   "Deep indent lists in parenthesis when non-nil.
 Also ignores spaces after parenthesis when `space'.
@@ -351,7 +358,7 @@
              (exp "and" exp) (exp "or" exp))
        (exp  (exp1) (exp "," exp) (exp "=" exp)
              (id " @ " exp)
-             (exp "." id))
+             (id "." exp))
        (exp1 (exp2) (exp2 "?" exp1 ":" exp1))
        (exp2 ("def" insts "end")
              ("begin" insts-rescue-insts "end")
@@ -399,7 +406,8 @@
        (nonassoc ">" ">=" "<" "<=")
        (nonassoc "==" "===" "!=")
        (nonassoc "=~" "!~")
-       (left "<<" ">>"))))))
+       (left "<<" ">>")
+       (assoc "."))))))
 
 (defun ruby-smie--bosp ()
   (save-excursion (skip-chars-backward " \t")
@@ -609,7 +617,18 @@
         ;; When after `.', let's always de-indent,
         ;; because when `.' is inside the line, the
         ;; additional indentation from it looks out of place.
-        ((smie-rule-parent-p ".") (smie-rule-parent (- ruby-indent-level)))
+        ((smie-rule-parent-p ".")
+         (let (smie--parent)
+           (save-excursion
+             ;; Traverse up the parents until the parent is "." at
+             ;; indentation, or any other token.
+             (while (and (progn
+                           (goto-char (1- (cadr (smie-indent--parent))))
+                           (not (ruby-smie--bosp)))
+                         (progn
+                           (setq smie--parent nil)
+                           (smie-rule-parent-p "."))))
+             (smie-rule-parent))))
         (t (smie-rule-parent))))))
     (`(:after . ,(or `"(" "[" "{"))
      ;; FIXME: Shouldn't this be the default behavior of
@@ -622,7 +641,10 @@
        (unless (or (eolp) (forward-comment 1))
          (cons 'column (current-column)))))
     (`(:before . "do") (ruby-smie--indent-to-stmt))
-    (`(:before . ".") ruby-indent-level)
+    (`(:before . ".")
+     (if (smie-rule-sibling-p)
+         (and ruby-align-chained-calls 0)
+       ruby-indent-level))
     (`(:after . "=>") ruby-indent-level)
     (`(:before . ,(or `"else" `"then" `"elsif" `"rescue" `"ensure"))
      (smie-rule-parent))

=== modified file 'test/automated/ruby-mode-tests.el'
--- test/automated/ruby-mode-tests.el	2014-01-01 07:43:34 +0000
+++ test/automated/ruby-mode-tests.el	2014-01-30 04:15:02 +0000
@@ -333,6 +333,20 @@
      |      42
      |    end")))
 
+(ert-deftest ruby-align-chained-calls ()
+  (let ((ruby-align-chained-calls t))
+    (ruby-should-indent-buffer
+     "one.two.three
+     |       .four
+     |
+     |my_array.select { |str| str.size > 5 }
+     |        .map    { |str| str.downcase }"
+     "one.two.three
+     |  .four
+     |
+     |my_array.select { |str| str.size > 5 }
+     |   .map    { |str| str.downcase }")))
+
 (ert-deftest ruby-move-to-block-stops-at-indentation ()
   (ruby-with-temp-buffer "def f\nend"
     (beginning-of-line)

=== modified file 'test/indent/ruby.rb'
--- test/indent/ruby.rb	2014-01-17 03:15:02 +0000
+++ test/indent/ruby.rb	2014-01-30 04:15:30 +0000
@@ -257,8 +257,8 @@
   bar
 
 foo_bar_tee(1, 2, 3)
-  .qux
-  .bar
+  .qux.bar
+  .tee
 
 foo do
   bar
@@ -348,7 +348,7 @@
 
 zoo
   .lose(
-  q, p)
+    q, p)
 
 foo(bar:
       tee)

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 16593 <at> debbugs.gnu.org
Subject: Re: bug#16593: 24.3.50;
 ruby-mode: align chained method calls on multiple lines
Date: Thu, 30 Jan 2014 09:35:37 -0500

> Warning (smie): Conflict: . </= .
> Warning (smie): Conflict: . </= .

The smie-precs->prec2 part of your grammar says that "." = "." (because
of the new (assoc ".") you add there).

But the BNF part of your grammar says "." < "." (i.e. "foo . bar . baz"
is parsed as "foo . (bar . baz)" because of (id "." exp).
Apparently there is no more conflict in the BNF in this respect so the
disambiguation constraint (assoc ".") that's passed to it is ignored.

Hence the conflict.


        Stefan

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 16593 <at> debbugs.gnu.org, Bozhidar Batsov <bozhidar <at> batsov.com>
Subject: Re: bug#16593: 24.3.50; ruby-mode: align chained method calls on
 multiple lines
Date: Fri, 31 Jan 2014 05:42:14 +0200

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

On 30.01.2014 16:35, Stefan Monnier wrote:
>> Warning (smie): Conflict: . </= .
>> Warning (smie): Conflict: . </= .
>
> The smie-precs->prec2 part of your grammar says that "." = "." (because
> of the new (assoc ".") you add there).
>
> But the BNF part of your grammar says "." < "." (i.e. "foo . bar . baz"
> is parsed as "foo . (bar . baz)" because of (id "." exp).

Fantastic, thanks for the explanation. With (assoc ".") changed to 
(right "."), the warning is gone.

I've also fixed the precedence of "." vs "? :" and modified a test case 
respectively. See the new patch attached.

Do you think it's not too late in the feature freeze to install a change 
like this, or should we wait until the trunk reopens?

We have a decent number of test cases, so it's not like the patch is 
likely to introduce major breakage.

> Apparently there is no more conflict in the BNF in this respect so the
> disambiguation constraint (assoc ".") that's passed to it is ignored.

Without that constraint there, SMIE showed warnings about ambiguities 
between ".", "," and "=". It somehow ignored the associativity, though.

The warnings with the previous patch also were gone when only the second 
one (assoc ".") were changed to (right "."). The first one, in the BNF 
disambiguation part, could be either.

Bozhidar, would you care to test the new patch?

[ruby-chained-calls.diff (text/x-patch, attachment)]

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

From: Bozhidar Batsov <bozhidar <at> batsov.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 16593 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#16593: 24.3.50; ruby-mode: align chained method calls
 on multiple lines
Date: Fri, 31 Jan 2014 15:31:39 +0200

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

Tested the patch locally and it works perfectly. IMO this patch should definitely be installed now, given its small scope and the fact it fixes problems in the current code (as well as providing an important customization option). 

-- 
Cheers,
Bozhidar


On Friday, January 31, 2014 at 5:42 AM, Dmitry Gutov wrote:

> On 30.01.2014 16:35, Stefan Monnier wrote:
> > > Warning (smie): Conflict: . </= .
> > > Warning (smie): Conflict: . </= .
> > > 
> > 
> > 
> > The smie-precs->prec2 part of your grammar says that "." = "." (because
> > of the new (assoc ".") you add there).
> > 
> > But the BNF part of your grammar says "." < "." (i.e. "foo . bar . baz"
> > is parsed as "foo . (bar . baz)" because of (id "." exp).
> > 
> 
> 
> Fantastic, thanks for the explanation. With (assoc ".") changed to 
> (right "."), the warning is gone.
> 
> I've also fixed the precedence of "." vs "? :" and modified a test case 
> respectively. See the new patch attached.
> 
> Do you think it's not too late in the feature freeze to install a change 
> like this, or should we wait until the trunk reopens?
> 
> We have a decent number of test cases, so it's not like the patch is 
> likely to introduce major breakage.
> 
> > Apparently there is no more conflict in the BNF in this respect so the
> > disambiguation constraint (assoc ".") that's passed to it is ignored.
> > 
> 
> 
> Without that constraint there, SMIE showed warnings about ambiguities 
> between ".", "," and "=". It somehow ignored the associativity, though.
> 
> The warnings with the previous patch also were gone when only the second 
> one (assoc ".") were changed to (right "."). The first one, in the BNF 
> disambiguation part, could be either.
> 
> Bozhidar, would you care to test the new patch? 
> 
> 
> Attachments: 
> - ruby-chained-calls.diff
> 

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

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 16593 <at> debbugs.gnu.org, Bozhidar Batsov <bozhidar <at> batsov.com>
Subject: Re: bug#16593: 24.3.50;
 ruby-mode: align chained method calls on multiple lines
Date: Fri, 31 Jan 2014 09:33:11 -0500

> Do you think it's not too late in the feature freeze to install a change
> like this, or should we wait until the trunk reopens?

It's up to you.

>> Apparently there is no more conflict in the BNF in this respect so the
>> disambiguation constraint (assoc ".") that's passed to it is ignored.
> Without that constraint there, SMIE showed warnings about ambiguities
> between ".", "," and "=". It somehow ignored the associativity, though.

That's normal.

The disambiguation constraints provide a bunch of constraints, and but only
the ones that resolve ambiguities are used.  The (assoc ".") being
together with other things, indicates not only that "." should be
associative (in case of ambiguity) but also its relative precedence
w.r.t other elements like , and =.


        Stefan

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Bozhidar Batsov <bozhidar <at> batsov.com>
Cc: 16593 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#16593: 24.3.50; ruby-mode: align chained method calls on
 multiple lines
Date: Fri, 31 Jan 2014 19:04:14 +0200

On 31.01.2014 15:31, Bozhidar Batsov wrote:
> Tested the patch locally and it works perfectly. IMO this patch should
> definitely be installed now, given its small scope and the fact it fixes
> problems in the current code (as well as providing an important
> customization option).

Thanks for checking!

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

From: Glenn Morris <rgm <at> gnu.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 16593 <at> debbugs.gnu.org, Bozhidar Batsov <bozhidar <at> batsov.com>
Subject: Re: bug#16593: 24.3.50;
 ruby-mode: align chained method calls on multiple lines
Date: Fri, 31 Jan 2014 12:18:02 -0500

The first line of doc strings should be complete sentences.
New defcustoms need :version tags.
Perhaps this needs a NEWS entry.

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 16593 <at> debbugs.gnu.org, Bozhidar Batsov <bozhidar <at> batsov.com>
Subject: Re: bug#16593: 24.3.50; ruby-mode: align chained method calls on
 multiple lines
Date: Fri, 31 Jan 2014 19:33:55 +0200

On 31.01.2014 19:18, Glenn Morris wrote:
>
> The first line of doc strings should be complete sentences.
> New defcustoms need :version tags.
> Perhaps this needs a NEWS entry.

Right, sorry. Fixed.

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 16593 <at> debbugs.gnu.org, Bozhidar Batsov <bozhidar <at> batsov.com>
Subject: Re: bug#16593: 24.3.50; ruby-mode: align chained method calls on
 multiple lines
Date: Fri, 31 Jan 2014 19:34:13 +0200

On 31.01.2014 16:33, Stefan Monnier wrote:
>> Do you think it's not too late in the feature freeze to install a change
>> like this, or should we wait until the trunk reopens?
>
> It's up to you.

Ok, done!

> The disambiguation constraints provide a bunch of constraints, and but only
> the ones that resolve ambiguities are used.  The (assoc ".") being
> together with other things, indicates not only that "." should be
> associative (in case of ambiguity) but also its relative precedence
> w.r.t other elements like , and =.

Thanks for the explanation.

Previous Next

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