-
Notifications
You must be signed in to change notification settings - Fork 162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Fix #164] Indentation of blocks inside call arguments #169
Conversation
The offending behaviour is now fixed: list(arg1 = {
stuff
},
arg2 = {
stuff
}) |
Thanks. Let's see what others have to say. I personally think this might be the best way to go given that's it's not a very common case and it's consistent (this time) with R-studio. |
(eq (char-after last-sexp) ?{) | ||
(and | ||
;; in case we have additional non-block arguments | ||
(eq (char-before last-sexp) ?\C-j) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these contextual checks for characters are a bit too specific. But If it works now I guess we can improve this later if we see any problems with it.
Could you please make this the default:
list(arg1 = {
stuff
}, arg2 = {
stuff
})
How about ess-indent-function-block-arguments
as an option name? To me it sounds a bit more informative than
ess-indent-block-arg-function`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a perfect name.
About the testing scheme, I'm trying to set up different R files containing the same testing set but with different configurations. To achieve this I tried to specify different file-local variables with the indentation settings. This doesn't work and indeed in the source code of (ess-mode)
we have this (kill-all-local-variables)
nuking all the variables I'm trying to set (IIUC).
I'm now trying to hack something by setting locally a modified ess-local-customize-alist
, but is there a canonical way to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To achieve this I tried to specify different file-local variables with the indentation settings.
Why do you need that? Are you using ert?
I was thinking about a very simple scheme:
(ert-deftest test-ess-RRR-indentaton ()
(find-file-noselect "tests/indent-RRR.R" t)
(ess-set-style 'RRR)
(indent-region (poitn-min) (point-max))
(should (not (buffer-modified-p))))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To bind custom stiles you can do something along:
(let ((ess-stile-alist
'(style
(ess-indent-level . 4)
(ess-first-continued-statement-offset . 2)
(ess-continued-statement-offset . 0)
(ess-brace-offset . -4)
(ess-expression-offset . 4)
(ess-else-offset . 0)
(ess-close-brace-offset . 0)
(ess-brace-imaginary-offset . 0)
(ess-continued-brace-offset . 0)
(ess-arg-function-offset . 4)
(ess-arg-function-offset-new-line . '(4))
)))
(ess-set-style 'style)
....
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I'll look into ert.
I was thinking of simple R files to load, with file-local vars automatically set. Then we'd indent them and git diff them to check nothing has changed.
but it would be much better if it could be more automatic!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ERT looks great :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks. If I can't finish this tonight this'll have to wait until Friday though.
In the mean time I'm going to open an issue with the test cases I already collected, so people can complete it.
Good we slept on this one for a while. I have just stumbled on the following annoyance:
which is considerably uglier than the old default
The worst part is with examples like the following:
We are already treating @lionel- If you don't have anything else to add, please squash everything into one commit and I will merge. |
I think we should merge the testing framework first, it's finished. It allowed me to discover that there is a bug in this PR with closing braces: ## 11
fun <- fun_call({
stuff
}, {
stuff
},
{
stuff
}
)
## 14
fun_call( {
body
}
) |
Is this issue that RStudio provides this indentation:
when this form would be preferred (when vertical alignment is turned on):
Honestly, given that 'style' I am not sure if there is an indentation that everyone could agree on, e.g. I would prefer to write with each argument on a separate line
which I imagine both ESS and RStudio would agree is the 'right' indentation. IMHO, the best indentation for the original example (with vertical alignment on) would be something like:
which is at least 'consistent'. But all forms of this indentation (where |
On the other hand Kevin, imagine how Maybe opinions on this matter depend on the programmer's setup. If you work on a laptop with the screen splitted in two, you can't afford wasting horizontal spacing for lambdas and There are no easy answer here so maybe making it a setting is best. |
Very good point! I agree entirely. |
0b8bece
to
1be3fd0
Compare
I fixed all remaining bugs and now To see it, put your cursor on the opening paren in the following snippet and try to call object[(
index)] Now I don't know if we want to fix that by defining our own sexp motion functions. Anyway, I realised that since the open paren is not a function call, fun_call(parameter = (
stuff
), Now you have: fun_call(parameter = (
stuff
) This makes sense because Also I noticed that ESS does not consider back-quoted names as R symbols and fixed that. Finally, the last commit implements a setting to turn indentation of block arguments on. It is very easy to implement and works well (see new test file). |
I think we should use the same indentation on open parenthesis irrespective if it's a function call or not. Moreover, I think we should do the same on
Instead of adding new offsets, I would rename
That's how |
it's an old bug, but you need to set I'll try to circumvent the |
By the way I'm working on roxygen indentation and autofilling code now. Can I use the new advice system of Emacs 24.4? Or should the new ESS releases be compatible with old Emacs versions? |
We are trying to support 24 entirely and ESS apparently works fine with emacs 23. So let's keep the old system for now. The advice system is easy to change, so we will do it in due time. |
I'm now trying to figure out the corner cases. The documentation of object[(
index)] we'd have: object[(
index)] That's not ideal but the indentation you want will still be available by setting
We are not allowed those characters in elisp names but it is indeed a good idea to make the settings more self explicit. We could also rename Also, since we have all those options pushing the indentation towards the left margin (block arguments, but also the {
fun_call(argument,
argument, {
stuff
}
,
argument, argument,
argument)
} However it is not sufficient to just check the indentation of the previous line because in many situations it will be over-indented compared to its "natural" indentation. Thus the easiest and most reliable way to find out the right indentation level is to climb over all arguments of the function call and use the minimum indentation found on the way. I implemented such an argument climber and the strategy seems to work well. |
Yes. That makes a lot of sense.
Why? The point of this setting is to indent ( and [ the same if they are ate the end of the line.
Of course:) Lisp is so permisive that at some point you think that everything is allowed.
I agree.
We will alias and deprecate those.
Are you sure? Which example do you have in mind? If last arg declaration ended in }, it should be fine. It worked correctly a couple of days ago, right? |
Here is one example where looking at the previous line does not work: {
fun_call(argument, {
stuff
},
parameter = fun_argument(arg, # this one! <--
sub_fun(arg, # not this one
param = sub_fun(arg, # not this one
arg # not this one
), # not this one
arg # not this one
), arg # not this one
), arg, # not this one
my_argument, # --> Which previous line informs the indentation?
argument,
argument, argument,
argument)
} What about renaming And ## ess-offset-arguments-after-hard-return = 4
fun_call1(argument = fun_call2(
subarg
) # closing parents does not go further than minimum indent
)
## ess-offset-arguments = 4
fun_call1(argument = fun_call2(subarg1,
subarg2
)
) And it would be ignored in all following situations: ## ess-offset-arguments-after-hard-return = '(4)
fun_call1(argument = fun_call2(
subarg
)
)
## ess-offset-arguments = '(4)
fun_call1(argument = fun_call2(subarg1,
subarg2
)
)
## ess-offset-arguments = nil
fun_call1(argument = fun_call2(subarg1,
subarg2
)
)
Should we settle for the closest opening delim attached to a name then? {
fun_call(argument, (
( # indented from fun_call(,
( # the closest named opening delim
object[(
(
index # indented from object[
)
)
]
)
)
))
} What about parentheses at top level? (
ggplot(...) +
geom(...)
) These are all weird corner cases but I think it's still important to get them consistently indented. This is the only way to avoid weird indentations in complicated situations we did not think about. |
Yes. I like all of the above proposals, but can we make
Ok. Now I see your point. I don't think that's a good idea. How about we check if open delims are grouped at the end of the line and indent according to the leftmost. If leftmost is not lonely they we respect
Would this work? |
I think I found a way to simplify things further and make the indentation logic more consistent at the same time. First, we change the behaviour of
## ess-offset-arguments = nil
## ess-offset-closing-paren = 0
{
fun_call(argument1, fun_call(subarg1,
subarg2
),
argument2
)
} ## ess-offset-arguments = 4
## ess-offset-closing-paren = 0
{
fun_call(argument1, fun_call(subarg1,
subarg2
),
argument2
)
} ## ess-offset-arguments = '(4)
## ess-offset-closing-paren = 0
{
fun_call(argument1, fun_call(subarg1,
subarg2
),
argument2 # pushed off from its natural indentation
)
} |
I think so. In the second example, we treat the lone |
Yes. I think the simplification that you proposed is the way to go. On this occasion we can rename all our offsets to start with All these changes make my head reel. I am losing track already. You will have to document all the changes in |
How about treating all open delimiters the same? We can drop a bunch of offsets then ( |
yes it's really hairy but I think these changes will make things clearer.
And have them all behave as a function of
yes please! Never understood the purpose of this setting. I also don't understand what And could we get rid of |
To be more clear, |
stuff | ||
), | ||
argument) | ||
argument1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one doesn't seem to be right. Do you indent with indent-tabs-mode
set to nil
? Github seems to screw tab indentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is actually right. But do we want to push the inner call argument all the way to beginning of the previous line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do. This is the only way to be consistent and to me it looks better this way.
Here we have inner arguments pushed off to the last indentation (here, the zeroth column) because the arguments-newline
is '(t)
. Since the closing delimiter offset is 0 (which will soon be hardcoded), we go all the way to bol
at the next line.
But it wouldn't be hard to make the pushing behaviour a setting if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it wouldn't be hard to make the pushing behaviour a setting if you want.
I thought that you already have a setting to indent arguments of the inner call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it's ess-offset-arguments
set to t
. Should this be the default for RRR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have ess-indent-from-outer-parameter
which seems not be respected here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because this one only makes sense when the offset type is t
, as opposed to '(t)
(as in here) or nil
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I be clearer about this in the doc string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I think I finally started getting it. So arguments-newline
take precedence everywhere right? So it's only when you keep an argument on the same line then ess-indent-from-outer-parameter
kicks in. That's indeed consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more or less :)
I rewrote the docstring:
This setting only has an effect when indentation of arguments is
relative to the innermost function call. That is, when
ess-offset-arguments or ess-offset-arguments-newline is set to a
number N as opposed to a nil value or a list '(N).
I suggest that after the I am also not sure that keeping Your intent was to make all styles to pick up the default |
But it is part of styles. Each style sets its own indent level. |
Ok. Then let's not change the default Seems like I got confused with the new |
Fixes emacs-ess#158, fixes emacs-ess#164, fixes emacs-ess#166 and fixes emacs-ess#167
argument2 | ||
) | ||
, | ||
argument3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well. Take this example. Right now (because ess-offset-arguments
= nil, and ess-offset-arguments-newline
= (t)
) I get either this
fun_call(parameter = fun_argument(argument1,
argument2
)
or this
fun_call(parameter = fun_argument(
argument1,
argument2
)
I cannot get anything in between because outer-parameter
setting is ignored.
So my concern is three fold. First, after {
this makes sense, does it really make sense after (
given that it's a very common pattern and all subsequent arguments will be shifted to 0. Second, it's not the current behavior, and folks will complain. Third, is there a way to make it
fun_call(parameter = fun_argument(
argument1,
argument2
)
without affecting the indentation after last {
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To get the last behaviour you set ess-offset-arguments-newline
to t
instead of '(t)
. Should it be the default for RRR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be nice if we had ouuter-parameter
as follows:
If nil, no effect at all (other variables set the indent).
If t
,
some_function(parameter = other_function(
argument
))
If (t)
:
some_function(parameter = other_function(
argument
))
And having it take precedence over offset-argument
and offset-argument-newline
unconditionally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that indentation after {
is controlled by ess-offset-block
and is independent of all this. ess-indent-from-outer-parameter
also has an effect for blocks, which I forgot to mention in the doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To get the last behaviour you set ess-offset-arguments-newline to t instead of '(t). Should it be the default for RRR?
But does this affect indentation of {
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm... I don't think it's a good idea. I don't see where the problem is with how things are currently? You can get the behaviour you want by setting ess-offset-arguments-newline
to t
and ess-indent-from-outer-parameter
to t
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have three main settings: blocks, arguments, and arguments after a newline. With your proposition, setting outer-parameter
to t
would override all three to be t
. We loose a lot of flexibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But does this affect indentation of {?
No, this is controlled independently by ess-offset-block
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if iindentatioin of { is not affected then I am completely fine with the current situation. I just thought that you took my older suggestion of makeing all open delimiters behave identically litteraly.
The only thing that I disagree is the new defaults for RRR. It should be very close to the current one to avoid disturbing people. So yes, setting ess-offset-arguments-newline to t is the way to go.
here you go @vspinu. The styles look a lot tidier now that those delimiter offsets are gone :) |
Major indentation rewrite Fix #164
Ok. So I guess I hurried a bit up. The stuff breaks a couple of cases in For instance (with RRR) style:
|
alright I'll take a look |
@vspinu my.long.Expression <- expression(
x[a[j]] == exp(theta[1] + theta[2]^2),
x[b[i]] == sin(theta[3] ~~ theta[4])
) but now with my.long.Expression <- expression(
x[a[j]] == exp(theta[1] + theta[2]^2),
x[b[i]] == sin(theta[3] ~~ theta[4])
) One way out would be to implement an idea that I had for the future, which is to allow different styles depending on the depth of embeddedness of expressions. For example we could decide to have object <- fun_call(
arg1,
arg2
) and object <- fun_call(
arg1, other_call(
arg2,
arg3
),
arg4) Under this scheme we'd solve the problem above. What do you think? |
I think we need to change RRR style a bit. Currently we have non-null The intention of RRR is to mimic common R style. R inferior indents like
instead of what we do
RStudio also indents like the first case above. We also want to minimize unnecessary differences introduced by comments. Currently comments are not respecting
I think this is so bad that I doubt we should use So, if there are no objections I would like to change that. |
I'm even wondering if there are any happy users of this continuation style? |
Yeh. In retrospect, we should have changed it when the predecessor of There is one last thing that I am not completely happy about. I would like a bit nicer names for I have made some cleanups and renaming. The whole system is now documented in |
hmm, not since the generalisation of this setting to assignments with As for the other setting, it's hard to find another name, especially if we need to keep the |
I think it is really good to think about good names for these. |
Why not? Those re still arguments what are aligned. In any case, you are probably right that keeping this one generic will probably serve us well in the future. So let's keep this one as it is. @mmaechler should also like the short name :)
We need
How about |
or should it be |
Then it should be So in the first case ( |
you're right, sounds good to me then. |
Since you don't want to add a new setting, I simply replaced the whole
(progn)
with(current-indentation)
.But this restores this behaviour: