RemoveScala3OptionalBraces: handle fewer braces #3815
Conversation
kitbellew
force-pushed
the
3812
branch
4 times, most recently
from
0d26ecd
to
fc0c020
Compare
docs/configuration.md
Outdated
| @@ -3698,6 +3698,13 @@ The section contains the following settings (available since v3.8.1): | |||
| - other flags below might extend rewrites to other cases | |||
| - `oldSyntaxToo` | |||
| - if `true`, applies also to expressions using deprecated syntax | |||
| - `fewerBracesMinStats` and `fewerBracesMaxStats` | |||
There was a problem hiding this comment.
Let's maybe add some info about the defaults? I assume it's 0?
There was a problem hiding this comment.
in general, it would have been ideal to count the number of lines and not the statements. however, this is performed before the formatting, and the number of output lines is not known at that point.
counting the number of input lines will lead to non idempotent formatting, as that will change. but the tree won't change, and so the number of statements.
in general, i was trying to avoid rewriting very short blocks, e.g. foo { bar }, which might look better on one line, especially since enabled redundant braces actually causes that to be charged to parens after the formatting, hence the minimum statements parameter. but we could also permanently fix it to, say, 2, if we can figure out how to exclude invisible blocks, and remove the parameter.
but the biggest one is the maximum. i think that rewriting a block which is way too large might make the code unreadable. but one can argue that in that case the code was unreadable to begin with, since very large blocks should have been refactored as separate methods.
what do you think? here are the two options, specifically:
- keep it very configurable, with both parameters; in that case, why wouldn't we count cases or other nested blocks?
- keep it simple, replace both parameters with a single boolean, determine minimum size ourselves and do not restrict maximum, if that's how the function argument is written?
| } | ||
| val delta = next match { | ||
| case x: Term.Block => enqueue(x.stats) | ||
| case x: Tree.WithBody => enqueue(x.body :: Nil) - 1 |
There was a problem hiding this comment.
as you can see here, -1 means that the parent expression is not counted and fully replaced by the statements in the body. this applies, for instance, to a function, which doesn't count the parameters.
| queue.length - len | ||
| } | ||
| val delta = next match { | ||
| case x: Term.Block => enqueue(x.stats) |
There was a problem hiding this comment.
this is where the block, including one without braces, is counted
| val delta = next match { | ||
| case x: Term.Block => enqueue(x.stats) | ||
| case x: Tree.WithBody => enqueue(x.body :: Nil) - 1 | ||
| case x: Tree.WithCases => enqueue(x.cases) |
There was a problem hiding this comment.
you mentioned not counting cases in a match statement. this line applies to partial functions as well, should we not count them either?
There was a problem hiding this comment.
i have decided to go with the same approach that is used for runner.optimizer.forceConfigStyleOnOffset parameter, by counting total span of non-whitespace tokens.
that was the only approach that is easy to describe, easy to implement and is not subject to non-idempotence problems (since we know which tokens are being removed during the rewrite session).
I would opt for that, since I can imagine people wanting fewer braces, but not at all possible cases, which without the additional configuration, would be very unfriendly to use. The only issue I see is that it should be obvious what the parameter does. And currently users would need to understand inner working of the parser to see what is going on. I would try to make it as simple as possible and only count statements inside the body of the block at the toplevel. |
docs/configuration.md
Outdated
| @@ -3698,6 +3698,13 @@ The section contains the following settings (available since v3.8.1): | |||
| - other flags below might extend rewrites to other cases | |||
| - `oldSyntaxToo` | |||
| - if `true`, applies also to expressions using deprecated syntax | |||
| - `fewerBracesMinNonWsSpan` and `fewerBracesMaxNonWsSpan` | |||
There was a problem hiding this comment.
| - `fewerBracesMinNonWsSpan` and `fewerBracesMaxNonWsSpan` | |
| - `fewerBracesMinSpan` and `fewerBracesMaxSpan` |
I would try to make it a bit less explicit, if anyone needs to check what is exactly being done they will anyway have to check the docs.
There was a problem hiding this comment.
thank you, done!
In some cases, invoke this rule from redundant-braces, since that rule is guaranteed to run before remove-optional-braces.
Fixes #3812.