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.

Sign up for GitHub

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

Jump to bottom

RedundantBraces: remove nested braces in lambda #1549

Merged
merged 2 commits into from
Nov 22, 2019
Merged

RedundantBraces: remove nested braces in lambda #1549

merged 2 commits into from
Nov 22, 2019

Conversation

kitbellew
Copy link
Collaborator

@kitbellew kitbellew commented Nov 11, 2019 •
edited
Loading

@kitbellew
Copy link
Collaborator Author

@olafurpg @tanishiking my first submission, please take a look.

I am also working on another but will likely need some guidance, can't figure out splits/policies/decisions. I will send a draft for comments shortly.

@kitbellew kitbellew mentioned this pull request Nov 11, 2019
Merged
@poslegm
Copy link
Collaborator

poslegm commented Nov 12, 2019

LGTM 👍

I want to test it for regressions on large codebase with #1551. I'll try to do it by the end of the week.

@tanishiking tanishiking mentioned this pull request Nov 13, 2019
Merged
@kitbellew
Copy link
Collaborator Author

@poslegm @tanishiking thanks for taking a look. On my side, I tested on scala-repos, by adding rewrite.rules = [RedundantBraces] to its .scalafmt.conf and running a binary built from the master first, then using the new one on it as well. There were no surprises after this second iteration.

olafurpg
olafurpg requested changes Nov 17, 2019
View reviewed changes
Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

Thank you @kitbellew Only one blocking comment, otherwise LGTM 👍

Can you maybe link to a diff in scala-repos with the changes from this PR? We could share the diff in the release notes to give people a better understanding of the changes.

scalafmt-core/shared/src/main/scala/org/scalafmt/rewrite/RedundantBraces.scala Outdated Show resolved Hide resolved
kitbellew pushed a commit to kitbellew/scala-repos that referenced this pull request Nov 17, 2019
Changes due to scalafmt 1549
81433d7 
scalameta/scalafmt#1549
@kitbellew
Copy link
Collaborator Author

@olafurpg @tanishiking @poslegm here's a fork of scala-repos showing the differences:

https://github.com/kitbellew/scala-repos/commits/scalafmt-1549

@kitbellew kitbellew force-pushed the lambda-remove-nested-braces branch from 76d94ed to 1012a25 Compare November 17, 2019 22:12
@kitbellew kitbellew requested a review from olafurpg November 17, 2019 22:12
kitbellew
kitbellew commented Nov 17, 2019
View reviewed changes
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.

@olafurpg @tanishiking @poslegm

The example diffs can be found here: https://github.com/kitbellew/scala-repos/commits/scalafmt-1549

Apparently, if you add ?w=1 to a commit's url, it will ignore whitespace changes (of which there are a lot there).

scalafmt-core/shared/src/main/scala/org/scalafmt/rewrite/RedundantBraces.scala Outdated Show resolved Hide resolved
olafurpg
olafurpg requested changes Nov 18, 2019
View reviewed changes
Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

The diff in scala repos looks great!

scalafmt-core/shared/src/main/scala/org/scalafmt/rewrite/RedundantBraces.scala Outdated Show resolved Hide resolved
@kitbellew kitbellew force-pushed the lambda-remove-nested-braces branch from 1012a25 to a687053 Compare November 18, 2019 04:04
kitbellew
kitbellew commented Nov 18, 2019
View reviewed changes
scalafmt-core/shared/src/main/scala/org/scalafmt/rewrite/Rewrite.scala Outdated
@@ -15,6 +17,8 @@ case class RewriteCtx(
implicit val dialect = style.runner.dialect
def isMatching(a: Token, b: Token) =
matchingParens.get(TokenOps.hash(a)).contains(b)

val activeForEdition_2019_11: Boolean = style.activeFor(Edition(2019, 11))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as requested, created a field for this flag

@kitbellew kitbellew requested a review from olafurpg November 18, 2019 04:05
@kitbellew kitbellew force-pushed the lambda-remove-nested-braces branch from a687053 to 96ea0fb Compare November 19, 2019 21:23
kitbellew
kitbellew commented Nov 19, 2019
View reviewed changes
scalafmt-core/shared/src/main/scala/org/scalafmt/config/ScalafmtConfig.scala
@@ -192,6 +192,13 @@ case class ScalafmtConfig(
continuationIndent.callSite,
continuationIndent.defnSite
)

// Edition-specific settings below
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if we want to keep predefined edition flags (rather than comparing two editions for every token, as @olafurpg pointed out), it probably makes sense to put them here.

after we merge this, i will rebase #1551 as it uses editions as well.

kitbellew
kitbellew commented Nov 19, 2019
View reviewed changes
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.

@olafurpg @tanishiking @poslegm please take a look. let me know if there's anything else. i published a link to scala-repos diff earlier.

@kitbellew kitbellew force-pushed the lambda-remove-nested-braces branch from 96ea0fb to 128fdeb Compare November 21, 2019 01:01
@kitbellew
Copy link
Collaborator Author

@olafurpg @poslegm @tanishiking at the risk of sounding impatient, i wonder if there's anything else you'd like to see. i would like to merge this and #1551 first, and then work on the other changes (I have half a dozen in pipeline, with a couple already submitted for review).

poslegm
poslegm approved these changes Nov 22, 2019
View reviewed changes
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.

LGTM! Sorry for delay, I'm pretty busy at job now.

Thank you very much for your big effort! I will try to review the rest of your PRs soon.

@poslegm
Copy link
Collaborator

poslegm commented Nov 22, 2019 •
edited
Loading

@olafurpg I took the responsibility to merge this PR because yours requested changes have been implemented. If you have another remarks we can do it separately.

@poslegm poslegm closed this Nov 22, 2019
@poslegm poslegm reopened this Nov 22, 2019
@poslegm poslegm merged commit d4de0dc into scalameta:master Nov 22, 2019
@kitbellew kitbellew deleted the lambda-remove-nested-braces branch November 23, 2019 04:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poslegm poslegm poslegm approved these changes

@olafurpg olafurpg Awaiting requested review from olafurpg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kitbellew @poslegm @olafurpg