-
Notifications
You must be signed in to change notification settings - Fork 276
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
FormatOps: implement indentOperator.exemptScope #3082
Conversation
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 is one thing below that is off compared to our style guide. Otherwise it looks perfect. :)
foo( | ||
a && | ||
b, | ||
a && | ||
b | ||
) |
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.
Theses would also be
foo( | |
a && | |
b, | |
a && | |
b | |
) | |
foo( | |
a && | |
b, | |
a && | |
b | |
) |
Even when there are multiple parameters, the binary operator chain is alone in their actual parameter.
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.
what's an actual parameter?
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.
and if you don't need them to be enclosed (which one parameter gives you but multiple do not), why is being alone in the case body (which is what started this conversation) not included?
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.
An actual parameter is an argument at call site. As opposed to a formal parameter, which is the parameter at definition site.
and if you don't need them to be enclosed (which one parameter gives you but multiple do not), why is being alone in the case body (which is what started this conversation) not included?
Hum actually being alone in a case
body should be included as well, yes.
The "alone" condition is not really about being enclosed. It's about avoiding things like
val foo = {
doSomethingFirst()
thenReturnThis ||
that
}
because there it's extremely confusing.
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, now I'm confused. @ekrich, didn't you say that the recent change to infix as entire case body was not welcomed?
@sjrd any other cases? so it's not about having delimiters but being alone? then what about
if (...)
a &&
b
indent or not? it's alone in then
body. or method body? or yield body?
or
// infix retrieves a lambda, and it's alone
(foo get
bar)(arg)
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.
@sjrd any other cases? so it's not about having delimiters but being alone? then what about
if (...) a && b
indent or not? it's alone in
then
body. or method body? or yield body?
Our coding style would mandate {}
around the then branch (because it has more than one line; yes I know scalafmt won't do this for me ;-) ), so this is a case that cannot happen for us. If I go beyond our own style, and we accept this as valid, I think it should be considered alone in the then
body, yes. I think this is only way that it can decently generalize to braceless syntax anyway.
or
// infix retrieves a lambda, and it's alone (foo get bar)(arg)
Good question. I don't think I would get in that situation, given my coding habits. I don't have a good answer for this one. I would say do whatever is simpler to implement. 🤷♂️
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.
Our coding style would mandate
{}
around the then branch (because it has more than one line; yes I know scalafmt won't do this for me ;-) )...
i implemented this rewrite recently: https://scalameta.org/scalafmt/docs/configuration.html#inserting-braces
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.
Looks good from my perspective, though I have no stake at any particular style here.
aec1b19
to
befec62
Compare
current
topLevelOnly
was likely intended to implement https://github.com/scala-js/scala-js/blob/main/CODINGSTYLE.md#long-expressions-with-binary-operators but the result is not as described.the new
enclosedAlone
option attempts to do that instead, but needs confirmation from scalajs developers.