-
Notifications
You must be signed in to change notification settings - Fork 278
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
Another fix for verticalMultilineAtDefinitionSite
#711
Conversation
Fixes an issue where defaulted methods with called with () would get split.
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.
LGTM
@@ -435,7 +435,8 @@ class Router(formatOps: FormatOps) { | |||
val oneLinePerArg = OneArgOneLineSplit(open).f | |||
val paramGroupSplitter: PartialFunction[Decision, Decision] = { | |||
// Indent seperators `)(` by `indentSep` | |||
case Decision(t @ FormatToken(_, rp @ RightParen(), _), _) => | |||
case Decision(t @ FormatToken(_, rp @ RightParen(), _), _) | |||
if rp == lastParen || !isCallSite(owners(rp)) => |
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 owners (rp) == leftOwner should also work. For pathological cases, you could for example define a new method inside the default parameter value 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.
Good point, added a test and fixed it.
name: Name, | ||
code: String = Defaults.code, | ||
updatedAt: Instant = Instant.now(), | ||
user: User = new User { def someFunc(): RT = () }, |
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 have btw seen crazy code like this! iirc it was somewhere in twitter/summingbird ^_^
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 can gladly say I've never seen this; wasn't even sure it would parse :\
Yet another bug fix for
verticalMultilineAtDefinitionSite
. I suspect this should be the last one though.Fixes an issue where
verticalMultilineAtDefinitionSite
would incorrectly split defaulted parameters that called a method.ie: