Skip to content
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

#1627 [02]: Router: ignore line breaks for blocks #1772

Merged
merged 3 commits into from Mar 9, 2020

Conversation

kitbellew
Copy link
Collaborator

@kitbellew kitbellew commented Mar 8, 2020

While previously we were trying to keep blocks without line breaks if that's how they were originally input, now we actively attempt to take a multi-line representation and fold it into a single line (fold), or take a single-line representation and unfold it into multiple lines (unfold). In the [keep] case, we'll keep the line break if it was there before.

Fixes #271 #1002 #1043
Helps with #1627

scala-repos diffs:

@kitbellew
Copy link
Collaborator Author

@olafurpg @tanishiking @poslegm PTAL, 2nd of many #1627 PRs.

Copy link
Collaborator

@poslegm poslegm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great overall, but I have a few questions:

  1. Why keep adds newline before lambda argument?

Screen Shot 2020-03-09 at 2 33 56 AM

  1. How fold will work with braces -> parens replacement (RedundantBraces)? Can I expect this formatting:
<<<
xs.map { x =>
  println(x)
}
>>>
xs.map(x => println(x))

def getSingleLineLambdaDecision: Policy.Pf = {
val ok = !style.newlines.alwaysBeforeCurlyBraceLambdaParams &&
getSpaceAndNewlineAfterCurlyLambda(newlines)._1
if (ok) getSingleLineDecisionFor2019Nov else null
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am concerned about an increasing number of nulls here. It may be error-prone in the future maintaining. Does Option really beats perfomance in this case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably not. i can try to see if we can change to Option instead, the problem earlier was that Split takes a Policy, so i had to do policyOpt.orNull anyway.

Copy link
Collaborator Author

@kitbellew kitbellew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Why keep adds newline before lambda argument?

replied on the code line. however, it doesn't add, i ran older scalafmt on the original code, and then the new one also on the original code (not on the formatted output), the diff is the comparison.

  1. How fold will work with braces -> parens replacement (RedundantBraces)? Can I expect this formatting:
<<<
xs.map { x =>
  println(x)
}
>>>
xs.map(x => println(x))

yes. fold will turn into xs.map { x => println(x) } and then FormatWriter will output with parens.

def getSingleLineLambdaDecision: Policy.Pf = {
val ok = !style.newlines.alwaysBeforeCurlyBraceLambdaParams &&
getSpaceAndNewlineAfterCurlyLambda(newlines)._1
if (ok) getSingleLineDecisionFor2019Nov else null
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably not. i can try to see if we can change to Option instead, the problem earlier was that Split takes a Policy, so i had to do policyOpt.orNull anyway.

@@ -276,7 +290,8 @@ class Router(formatOps: FormatOps) {
Split(Space, 0)
.notIf(
style.newlines.alwaysBeforeCurlyBraceLambdaParams ||
isSelfAnnotation || lambdaPolicy == null
isSelfAnnotation || lambdaPolicy == null ||
(style.newlines.source eq Newlines.keep) && newlines != 0
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Why keep adds newline before lambda argument?

this is the line. if newline is present, the Space split is disabled. (of course, it's up to us to define what keep does.)

ah, one more thing which I should have mentioned: the scala-repos diffs are not generated by running one version of scalafmt and then another (because the next one can't undo what the first one did). instead, both ran on the same original, pre-scalafmt commit, and the diff is the comparison between results. that's how a space split becomes a newline in keep.

Copy link
Collaborator

@poslegm poslegm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. fold will turn into xs.map { x => println(x) } and then FormatWriter will output with parens.

It's really powerful thing 👍

@kitbellew kitbellew force-pushed the 1772 branch 2 times, most recently from 546510d to bee23c8 Compare March 9, 2020 02:06
While previously we were trying to keep blocks without line breaks if
that's how they were originally input, now we actively attempt to take a
multi-line representation and fold it into a single line (fold), or take
a single-line representation and unfold it into multiple lines (unfold).

Fixes scalameta#271 scalameta#1002 scalameta#1043
@kitbellew kitbellew merged commit 4b95cee into scalameta:master Mar 9, 2020
@kitbellew kitbellew deleted the 1772 branch March 9, 2020 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

} else
2 participants