* bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren
@ 2013-12-12 1:57 Dmitry Gutov
2013-12-12 12:55 ` Stefan Monnier
2013-12-14 8:23 ` Steve Purcell
0 siblings, 2 replies; 14+ messages in thread
From: Dmitry Gutov @ 2013-12-12 1:57 UTC (permalink / raw)
To: 16116
Try this example with ruby-mode:
foo(
a,
b => [
1, 3, 4
],
c => [
5, 6
])
Currently, it will indent the last line to the 0th column, which doesn't
look right to me.
The following patch fixes that. Would it be all right to install it?
=== modified file 'lisp/emacs-lisp/smie.el'
--- lisp/emacs-lisp/smie.el 2013-11-04 20:45:36 +0000
+++ lisp/emacs-lisp/smie.el 2013-12-12 01:39:59 +0000
@@ -1423,8 +1423,7 @@
(save-excursion
;; (forward-comment (point-max))
(when (looking-at "\\s)")
- (while (not (zerop (skip-syntax-forward ")")))
- (skip-chars-forward " \t"))
+ (forward-char 1)
(condition-case nil
(progn
(backward-sexp 1)
In GNU Emacs 24.3.50.8 (x86_64-unknown-linux-gnu, GTK+ Version 3.8.6)
of 2013-12-09 on axl
Bzr revision: 115440 dmantipov@yandex.ru-20131209163052-oess75ps2o5tt61q
Windowing system distributor `The X.Org Foundation', version 11.0.11403000
System Description: Ubuntu 13.10
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren
2013-12-12 1:57 bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren Dmitry Gutov
@ 2013-12-12 12:55 ` Stefan Monnier
2013-12-12 16:30 ` Dmitry Gutov
2013-12-14 8:23 ` Steve Purcell
1 sibling, 1 reply; 14+ messages in thread
From: Stefan Monnier @ 2013-12-12 12:55 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 16116
> Currently, it will indent the last line to the 0th column, which doesn't
> look right to me.
Well, it does look right to me, because that line closes the whole construct.
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren
2013-12-12 12:55 ` Stefan Monnier
@ 2013-12-12 16:30 ` Dmitry Gutov
0 siblings, 0 replies; 14+ messages in thread
From: Dmitry Gutov @ 2013-12-12 16:30 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 16116
On 12.12.2013 14:55, Stefan Monnier wrote:
> Well, it does look right to me, because that line closes the whole construct.
Here are some arguments on why it's wrong:
- It deforms the last array literal and breaks the symmetry with the
previous one.
- If it emphasizes the fact that the construct is closed, why doesn't it
align the opening and closing round parens, and aligns delimiters from
different sexps instead?
- What about the following example? The last line likewise closes the
whole construct, but we don't indent it to the 0th column, right?
foo(
a,
b,
c)
- Likewise, this example:
foo([a,
b])
- Indentation functions usually don't care about tokens that go after
the first one after indentation. I'm used to that. For example, take
js-mode. It handles the initial example differently, but compare this
one under both modes:
foo([a,
b
])
js-mode indents it as I would expect.
- If I want to emphasize the closing of the whole construct, I can put
the closing paren on a separate line.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren
2013-12-12 1:57 bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren Dmitry Gutov
2013-12-12 12:55 ` Stefan Monnier
@ 2013-12-14 8:23 ` Steve Purcell
2013-12-14 14:13 ` Stefan Monnier
1 sibling, 1 reply; 14+ messages in thread
From: Steve Purcell @ 2013-12-14 8:23 UTC (permalink / raw)
To: 16116
I agree strongly with Dmitry here. Column zero is fine if the paren is on its own, but if there are any preceding characters on the line which have a different logical indent level, as is the case with “]” here, the indentation for those should dominate.
As another example, in the case of javascript, there’s the idiomatic function case:
something(function() {
… blah
});
and I don’t think this is inconsistent with the above: unindenting the “}” relative to the function body will have the right effect without even needing to consider the “)”.
-Steve
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren
2013-12-14 8:23 ` Steve Purcell
@ 2013-12-14 14:13 ` Stefan Monnier
2013-12-14 15:04 ` Dmitry Gutov
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Monnier @ 2013-12-14 14:13 UTC (permalink / raw)
To: Steve Purcell; +Cc: 16116
> I agree strongly with Dmitry here.
No need to convince anyone here. I was just pointing out that what is
right will depend, so the right fix is to let the rules-function
control it, rather than to replace one hard-coded choice with another.
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren
2013-12-14 14:13 ` Stefan Monnier
@ 2013-12-14 15:04 ` Dmitry Gutov
2013-12-14 15:32 ` Stefan Monnier
0 siblings, 1 reply; 14+ messages in thread
From: Dmitry Gutov @ 2013-12-14 15:04 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 16116, Steve Purcell
Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
> No need to convince anyone here. I was just pointing out that what is
> right will depend
Sorry, I didn't get that.
> so the right fix is to let the rules-function
> control it, rather than to replace one hard-coded choice with another.
That won't work. ruby-smie-rules is never called because
smie-indent-close doesn't use smie-indent--rule, and it goes before the
functions that do in smie-indent-functions. Unless you're suggesting to
change either of those points.
I thought rather to add a defvar to switch between the two behaviors in
smie-indent-close, or make smie-indent-functions buffer-local and
replace smie-indent-close there with a modified implementation.
Still inclined toward the latter.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren
2013-12-14 15:04 ` Dmitry Gutov
@ 2013-12-14 15:32 ` Stefan Monnier
2013-12-14 18:25 ` Dmitry Gutov
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Monnier @ 2013-12-14 15:32 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 16116, Steve Purcell
> Unless you're suggesting to change either of those points.
That's exactly what I'm suggesting, yes.
> I thought rather to add a defvar to switch between the two behaviors in
> smie-indent-close,
That could be acceptable, tho so far all the indentation is controlled
by the rules-function, so it makes sense to keep it that way rather than
to introduce a variable. Also, if it's done in the rules-function, the
function may return different results depending on context.
> or make smie-indent-functions buffer-local and replace
> smie-indent-close there with a modified implementation.
This approach would work if the requirement is very specific to one
particular major mode, but in this case, it seems to be a fairly
common one.
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren
2013-12-14 15:32 ` Stefan Monnier
@ 2013-12-14 18:25 ` Dmitry Gutov
2013-12-15 1:50 ` Stefan Monnier
0 siblings, 1 reply; 14+ messages in thread
From: Dmitry Gutov @ 2013-12-14 18:25 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 16116, Steve Purcell
Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
> That's exactly what I'm suggesting, yes.
So, call `smie-indent--rule' from `smie-indent-close'? That should work.
>> I thought rather to add a defvar to switch between the two behaviors in
>> smie-indent-close,
>
> That could be acceptable, tho so far all the indentation is controlled
> by the rules-function, so it makes sense to keep it that way rather than
> to introduce a variable.
Maybe so.
> Also, if it's done in the rules-function, the
> function may return different results depending on context.
True, but so far I don't see a situation in Ruby where it'd depend on
the context.
>> or make smie-indent-functions buffer-local and replace
>> smie-indent-close there with a modified implementation.
>
> This approach would work if the requirement is very specific to one
> particular major mode, but in this case, it seems to be a fairly
> common one.
I believe this argument also works against doing it in the rules
function, and in favor of adding a defvar.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren
2013-12-14 18:25 ` Dmitry Gutov
@ 2013-12-15 1:50 ` Stefan Monnier
2013-12-15 2:44 ` Dmitry Gutov
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Monnier @ 2013-12-15 1:50 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 16116, Steve Purcell
>> Also, if it's done in the rules-function, the
>> function may return different results depending on context.
> True, but so far I don't see a situation in Ruby where it'd depend on
> the context.
Agreed, but we're talking about a change in smie.el, not in ruby-mode.el ;-)
>>> or make smie-indent-functions buffer-local and replace
>>> smie-indent-close there with a modified implementation.
>> This approach would work if the requirement is very specific to one
>> particular major mode, but in this case, it seems to be a fairly
>> common one.
> I believe this argument also works against doing it in the rules
> function, and in favor of adding a defvar.
I don't think so: setting the var is a one-liner, adding the rule to the
rule-function is also a one-liner. So either way is just as easy for
the major-mode.
By contrast, setting smie-indent-functions buffer-locally, then
removing smie-indent-close from it and adding some other function
requires a lot more code, and a lot more brittle as well (the ordering
in smie-indent-functions is important, the set of functions in there and
their order is not guaranteed to stay unchanged in future versions, the
replacement function needs to be written (delegating to
smie-indent-close seems like it might not work), ...
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren
2013-12-15 1:50 ` Stefan Monnier
@ 2013-12-15 2:44 ` Dmitry Gutov
2013-12-15 13:00 ` Stefan Monnier
0 siblings, 1 reply; 14+ messages in thread
From: Dmitry Gutov @ 2013-12-15 2:44 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 16116, Steve Purcell
On 15.12.2013 03:50, Stefan Monnier wrote:
>> I believe this argument also works against doing it in the rules
>> function, and in favor of adding a defvar.
>
> I don't think so: setting the var is a one-liner, adding the rule to the
> rule-function is also a one-liner. So either way is just as easy for
> the major-mode.
I mean in terms of code reuse: the rules function is also specific to a
major mode. The rule itself doesn't look like it'll take just one line
to me, AFAICS it'll have to duplicate most of the code in
`smie-indent-close':
(`(:before . ,(or `")" `"]" `"}"))
(save-excursion
(forward-char 1)
(condition-case nil
(progn
(backward-sexp 1)
(cons 'column . (smie-indent-virtual)))
(scan-error nil))))
If any other major mode wants to do the same, they have to duplicate
this, or extract this code to a helper function in smie.el.
Using the rules function will also add 2-3 lines to `smie-indent-close'.
> By contrast, setting smie-indent-functions buffer-locally, then
> removing smie-indent-close from it and adding some other function
> requires a lot more code, and a lot more brittle as well (the ordering
> in smie-indent-functions is important, the set of functions in there and
> their order is not guaranteed to stay unchanged in future versions, the
> replacement function needs to be written (delegating to
> smie-indent-close seems like it might not work), ...
Looks like three lines to me. :)
(setq-local smie-indent-functions (copy-sequence smie-indent-functions))
(setcar (memq 'smie-indent-close smie-indent-functions)
'ruby--smie-indent-close)
And the replacement function wouldn't be much longer than the added rule.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren
2013-12-15 2:44 ` Dmitry Gutov
@ 2013-12-15 13:00 ` Stefan Monnier
2013-12-15 23:22 ` Dmitry Gutov
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Monnier @ 2013-12-15 13:00 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 16116, Steve Purcell
>>> I believe this argument also works against doing it in the rules
>>> function, and in favor of adding a defvar.
>> I don't think so: setting the var is a one-liner, adding the rule to the
>> rule-function is also a one-liner. So either way is just as easy for
>> the major-mode.
> I mean in terms of code reuse: the rules function is also specific
> to a major mode. The rule itself doesn't look like it'll take just one line
> to me, AFAICS it'll have to duplicate most of the code in
> smie-indent-close':
No, the rule should be something like
(`(:close-all . ,_) t)
I.e. equivalent to
(setq-local smie-indent-close-all t)
-- Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren
2013-12-15 13:00 ` Stefan Monnier
@ 2013-12-15 23:22 ` Dmitry Gutov
2013-12-16 14:23 ` Stefan Monnier
0 siblings, 1 reply; 14+ messages in thread
From: Dmitry Gutov @ 2013-12-15 23:22 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 16116, Steve Purcell
[-- Attachment #1: Type: text/plain, Size: 412 bytes --]
Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
> No, the rule should be something like
>
> (`(:close-all . ,_) t)
Ah, ok. How does this look to you?
Note that it makes the new behavior the default one, because "close all"
sounds more fitting to characterize the current behavior ("closing" all
sexps ending on the current line).
If that's not what you meant, maybe a different method name is in order.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: smie-close-all.diff --]
[-- Type: text/x-diff, Size: 3044 bytes --]
=== modified file 'lisp/emacs-lisp/smie.el'
--- lisp/emacs-lisp/smie.el 2013-12-11 15:59:27 +0000
+++ lisp/emacs-lisp/smie.el 2013-12-15 23:09:35 +0000
@@ -1135,6 +1135,10 @@
- :list-intro, in which case ARG is a token and the function should return
non-nil if TOKEN is followed by a list of expressions (not separated by any
token) rather than an expression.
+- :close-all, in which case ARG is a close-paren token at indentation and
+ the function should return non-nil if it should be aligned with the opener
+ of the last close-paren token on the same line, if there are multiple.
+ Otherwise, it will be aligned with its own opener.
When ARG is a token, the function is called with point just before that token.
A return value of nil always means to fallback on the default behavior, so the
@@ -1316,8 +1320,8 @@
(defun smie-indent--rule (method token
;; FIXME: Too many parameters.
&optional after parent base-pos)
- "Compute indentation column according to `indent-rule-functions'.
-METHOD and TOKEN are passed to `indent-rule-functions'.
+ "Compute indentation column according to `smie-rules-function'.
+METHOD and TOKEN are passed to `smie-rules-function'.
AFTER is the position after TOKEN, if known.
PARENT is the parent info returned by `smie-backward-sexp', if known.
BASE-POS is the position relative to which offsets should be applied."
@@ -1330,11 +1334,7 @@
;; - :after tok, where
;; ; after is set; parent=nil; base-pos=point;
(save-excursion
- (let ((offset
- (let ((smie--parent parent)
- (smie--token token)
- (smie--after after))
- (funcall smie-rules-function method token))))
+ (let ((offset (smie-indent--rule-1 method token after parent)))
(cond
((not offset) nil)
((eq (car-safe offset) 'column) (cdr offset))
@@ -1355,6 +1355,12 @@
(smie-indent-virtual) (current-column)))))
(t (error "Unknown indentation offset %s" offset))))))
+(defun smie-indent--rule-1 (method token &optional after parent)
+ (let ((smie--parent parent)
+ (smie--token token)
+ (smie--after after))
+ (funcall smie-rules-function method token)))
+
(defun smie-indent-forward-token ()
"Skip token forward and return it, along with its levels."
(let ((tok (funcall smie-forward-token-function)))
@@ -1423,8 +1429,13 @@
(save-excursion
;; (forward-comment (point-max))
(when (looking-at "\\s)")
- (while (not (zerop (skip-syntax-forward ")")))
- (skip-chars-forward " \t"))
+ (if (smie-indent--rule-1 :close-all
+ (buffer-substring-no-properties
+ (point) (1+ (point)))
+ (1+ (point)))
+ (while (not (zerop (skip-syntax-forward ")")))
+ (skip-chars-forward " \t"))
+ (forward-char 1))
(condition-case nil
(progn
(backward-sexp 1)
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren
2013-12-15 23:22 ` Dmitry Gutov
@ 2013-12-16 14:23 ` Stefan Monnier
2013-12-17 3:03 ` Dmitry Gutov
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Monnier @ 2013-12-16 14:23 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 16116, Steve Purcell
> Ah, ok. How does this look to you?
I think it's OK.
> If that's not what you meant, maybe a different method name is in order.
Preserving the old behavior would maybe be better, but ... let's try it
as is.
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren
2013-12-16 14:23 ` Stefan Monnier
@ 2013-12-17 3:03 ` Dmitry Gutov
0 siblings, 0 replies; 14+ messages in thread
From: Dmitry Gutov @ 2013-12-17 3:03 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 16116-done, Steve Purcell
On 16.12.2013 16:23, Stefan Monnier wrote:
> I think it's OK.
Thanks for looking, applied.
> Preserving the old behavior would maybe be better, but ... let's try it
> as is.
I do believe the new behavior is a better default.
Octave, Prolog, etc, examples in test/indent seem unaffected, but there
is one affected example in elpa/packages/sml-mode/testcases.sml (I'm
also getting other mismatches there, but those are probably unrelated).
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-12-17 3:03 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-12 1:57 bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren Dmitry Gutov
2013-12-12 12:55 ` Stefan Monnier
2013-12-12 16:30 ` Dmitry Gutov
2013-12-14 8:23 ` Steve Purcell
2013-12-14 14:13 ` Stefan Monnier
2013-12-14 15:04 ` Dmitry Gutov
2013-12-14 15:32 ` Stefan Monnier
2013-12-14 18:25 ` Dmitry Gutov
2013-12-15 1:50 ` Stefan Monnier
2013-12-15 2:44 ` Dmitry Gutov
2013-12-15 13:00 ` Stefan Monnier
2013-12-15 23:22 ` Dmitry Gutov
2013-12-16 14:23 ` Stefan Monnier
2013-12-17 3:03 ` Dmitry Gutov
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).