* bug#16593: 24.3.50; ruby-mode: align chained method calls on multiple lines @ 2014-01-30 4:24 Dmitry Gutov 2014-01-30 14:35 ` Stefan Monnier 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Gutov @ 2014-01-30 4:24 UTC (permalink / raw) To: 16593 [-- Attachment #1: Type: text/plain, Size: 239 bytes --] 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: . </= . [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: ruby-chained-calls.diff --] [-- Type: text/x-diff, Size: 3655 bytes --] === 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) ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#16593: 24.3.50; ruby-mode: align chained method calls on multiple lines 2014-01-30 4:24 bug#16593: 24.3.50; ruby-mode: align chained method calls on multiple lines Dmitry Gutov @ 2014-01-30 14:35 ` Stefan Monnier 2014-01-31 3:42 ` Dmitry Gutov 0 siblings, 1 reply; 9+ messages in thread From: Stefan Monnier @ 2014-01-30 14:35 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 16593 > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#16593: 24.3.50; ruby-mode: align chained method calls on multiple lines 2014-01-30 14:35 ` Stefan Monnier @ 2014-01-31 3:42 ` Dmitry Gutov 2014-01-31 13:31 ` Bozhidar Batsov 2014-01-31 14:33 ` Stefan Monnier 0 siblings, 2 replies; 9+ messages in thread From: Dmitry Gutov @ 2014-01-31 3:42 UTC (permalink / raw) To: Stefan Monnier; +Cc: 16593, Bozhidar Batsov [-- Attachment #1: Type: text/plain, Size: 1349 bytes --] 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? [-- Attachment #2: ruby-chained-calls.diff --] [-- Type: text/x-patch, Size: 4299 bytes --] === 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-31 03:17:28 +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'. @@ -350,10 +357,10 @@ ;; but avoids lots of conflicts: (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") + (exp2 (exp3) (exp3 "." exp2)) + (exp3 ("def" insts "end") ("begin" insts-rescue-insts "end") ("do" insts "end") ("class" insts "end") ("module" insts "end") @@ -380,7 +387,7 @@ (ielsei (itheni) (itheni "else" insts)) (if-body (ielsei) (if-body "elsif" if-body))) '((nonassoc "in") (assoc ";") (right " @ ") - (assoc ",") (right "=") (assoc ".")) + (assoc ",") (right "=")) '((assoc "when")) '((assoc "elsif")) '((assoc "rescue" "ensure")) @@ -399,7 +406,8 @@ (nonassoc ">" ">=" "<" "<=") (nonassoc "==" "===" "!=") (nonassoc "=~" "!~") - (left "<<" ">>")))))) + (left "<<" ">>") + (right ".")))))) (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-31 02:55:31 +0000 @@ -257,8 +257,8 @@ bar foo_bar_tee(1, 2, 3) - .qux - .bar + .qux.bar + .tee foo do bar @@ -338,7 +338,7 @@ %^abc^ ddd -qux = foo ? +qux = foo.fee ? bar : tee @@ -348,7 +348,7 @@ zoo .lose( - q, p) + q, p) foo(bar: tee) ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#16593: 24.3.50; ruby-mode: align chained method calls on multiple lines 2014-01-31 3:42 ` Dmitry Gutov @ 2014-01-31 13:31 ` Bozhidar Batsov 2014-01-31 17:04 ` Dmitry Gutov 2014-01-31 14:33 ` Stefan Monnier 1 sibling, 1 reply; 9+ messages in thread From: Bozhidar Batsov @ 2014-01-31 13:31 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 16593 [-- Attachment #1: Type: text/plain, Size: 1820 bytes --] 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 > [-- Attachment #2: Type: text/html, Size: 3377 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#16593: 24.3.50; ruby-mode: align chained method calls on multiple lines 2014-01-31 13:31 ` Bozhidar Batsov @ 2014-01-31 17:04 ` Dmitry Gutov 2014-01-31 17:18 ` Glenn Morris 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Gutov @ 2014-01-31 17:04 UTC (permalink / raw) To: Bozhidar Batsov; +Cc: 16593 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! ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#16593: 24.3.50; ruby-mode: align chained method calls on multiple lines 2014-01-31 17:04 ` Dmitry Gutov @ 2014-01-31 17:18 ` Glenn Morris 2014-01-31 17:33 ` Dmitry Gutov 0 siblings, 1 reply; 9+ messages in thread From: Glenn Morris @ 2014-01-31 17:18 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 16593, Bozhidar Batsov The first line of doc strings should be complete sentences. New defcustoms need :version tags. Perhaps this needs a NEWS entry. ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#16593: 24.3.50; ruby-mode: align chained method calls on multiple lines 2014-01-31 17:18 ` Glenn Morris @ 2014-01-31 17:33 ` Dmitry Gutov 0 siblings, 0 replies; 9+ messages in thread From: Dmitry Gutov @ 2014-01-31 17:33 UTC (permalink / raw) To: Glenn Morris; +Cc: 16593, Bozhidar Batsov 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#16593: 24.3.50; ruby-mode: align chained method calls on multiple lines 2014-01-31 3:42 ` Dmitry Gutov 2014-01-31 13:31 ` Bozhidar Batsov @ 2014-01-31 14:33 ` Stefan Monnier 2014-01-31 17:34 ` Dmitry Gutov 1 sibling, 1 reply; 9+ messages in thread From: Stefan Monnier @ 2014-01-31 14:33 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 16593, Bozhidar Batsov > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#16593: 24.3.50; ruby-mode: align chained method calls on multiple lines 2014-01-31 14:33 ` Stefan Monnier @ 2014-01-31 17:34 ` Dmitry Gutov 0 siblings, 0 replies; 9+ messages in thread From: Dmitry Gutov @ 2014-01-31 17:34 UTC (permalink / raw) To: Stefan Monnier; +Cc: 16593, Bozhidar Batsov 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-01-31 17:34 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-01-30 4:24 bug#16593: 24.3.50; ruby-mode: align chained method calls on multiple lines Dmitry Gutov 2014-01-30 14:35 ` Stefan Monnier 2014-01-31 3:42 ` Dmitry Gutov 2014-01-31 13:31 ` Bozhidar Batsov 2014-01-31 17:04 ` Dmitry Gutov 2014-01-31 17:18 ` Glenn Morris 2014-01-31 17:33 ` Dmitry Gutov 2014-01-31 14:33 ` Stefan Monnier 2014-01-31 17:34 ` Dmitry Gutov
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.