-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Change fewerbraces to always use a colon, even before lambdas #15273
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
Conversation
1028f8a
to
a668041
Compare
:
:
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 feel like this proposal looks much cleaner that the previous approach. I left a few comments, but I would say the only blocking one is the issue with no whitespace allowed before :
: | ||
y => y.toString | ||
"hello" | ||
.apply: |
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 instead of single :
we need to explicitely call apply in case of curried functions? I think that is sensible
Instead of parsing all formal parameters of a lambda as expressions and converting to parameters at the end, we now scan ahead the first time we see a `:` to determine whether the list is followed by `=>` or `?=>`. If that's the case we parse this parameter and all following ones as bindings instead of expressions.
- Drop the criterion of no whitespace. - Also, refactor the logic so that tokens are converted to COLONeol only on demand.
Since we now convert to COLONeol only on demand, there's no need to conert back in `isColon`.
3bc1ff2
to
69c4ad5
Compare
Under 3.0-migration { x: T => expr } Is legal code but it's forbidden for later versions. If `fewerBraces` was the default, that code would be ambiguous.
I think the proximity with type ascriptions is too close: def f(x: Int)(y: Int): Int
def g(x: Int): Int => Int
// type ascription
f(42): Int => Int
// lambda argument
g(42): (x: Int) =>
x + 1
// inline lambda argument
g(42): (x: Int) => x + 1
// puzzle…
trait X:
type Y
def Y: Y
val k: (x: X) => x.Y // type ascription or lambda argument?
// even worse
def z(n: Int): (x: X) => x.Y
z(42): (x: X) => x.Y // type ascription or lambda argument? |
In Scala 2, we could not write: xs.foldLeft(0)((x, y) =>
val z = x + y
z + 1
) This is one of the reasons why it was convenient to use braces instead of the usual application with parens: xs.foldLeft(0) { (x, y) =>
val z = x + y
z + 1
} But now, even without I don’t think we should get rid of the parens, which are anyway required for “non-lambda” arguments. |
Co-authored-by: Julien Richard-Foy <[email protected]>
@julienrf Note that lambda arguments in the version must be multi-line with a |
I have probably hit 200 times this test instead of the real Parsers.scala when trying to open it. Enough!
We already allow `: indent` there. For consistency we should allow `: params => indent` as well.
…ng/dotty into change-fewerbraces-1
Drop strings and quoted idents. That leaves: - alphanumeric identifiers, backticked identifiers, this, super, `)`, `]` This choice was made to keep the extension minimal. Quite a few files in the intent project have to be changed since they all use an idiom like "Formatting": ... with a colon after a string literal. We can discuss separately whether we want to support this, but IMO there's no pressing need to do so.
Drop fewerbraces.md, it's now fully part of indentation.md.
6300df2
to
b1a6867
Compare
There was quite some breakage over time where one version but not the other was edited.
Fixes #15082 |
I think this is a clear win over what we had before. If there are no objections I am going to merge this tomorrow. |
So it's
But the following will still not work (it is however supported by #15258):