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

Proposal to use Scalafmt with the Scala files #4965

Merged
merged 1 commit into from
Jul 9, 2018
Merged

Proposal to use Scalafmt with the Scala files #4965

merged 1 commit into from
Jul 9, 2018

Conversation

joan38
Copy link
Contributor

@joan38 joan38 commented May 3, 2018 •
edited
Loading

This eliminates the different people's opinion on coding style and makes the codebase consistant.
I put arbitrary rules, let me know your thoughts.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@joan38 joan38 changed the title Scalafmt Scalafmt? May 3, 2018
@joan38 joan38 changed the title Scalafmt? Proposal to use Scalafmt May 4, 2018
@joan38 joan38 changed the title Proposal to use Scalafmt Proposal to use Scalafmt to the Scala files May 4, 2018
@joan38 joan38 changed the title Proposal to use Scalafmt to the Scala files Proposal to use Scalafmt with the Scala files May 4, 2018
@joan38
Copy link
Contributor Author

joan38 commented May 4, 2018

This is ready to be reviewed, the tests are failing because of misformatted files which I will resolve after I get all the feedbacks.

@joan38 joan38 mentioned this pull request May 9, 2018
Open
3 tasks
@joan38
Copy link
Contributor Author

joan38 commented May 10, 2018

As discussed on the mailing list https://lists.apache.org/thread.html/3afa79db248e7b6052a0e53bd5c5060e82412a0e83d06ba6dc5725e9@%3Cdev.kafka.apache.org%3E
We are looking into bringing the formatter piece by piece starting with the streams.

Please feel free to comment on formatting that feels weird.
Overall the goal is to settle some rules no matter what they are to avoid having to think about them when codding or reviewing PRs.

@joan38
Copy link
Contributor Author

joan38 commented May 11, 2018

@ijuma @guozhangwang @debasishg What are your thoughts on this? I understand from the mailing list that the streams module is good to go?

@debasishg
Copy link
Contributor

👍 for having this in streams module.

@ijuma
Copy link
Contributor

ijuma commented May 11, 2018

@joan38 the plan sounds good. I'll try to take a look at the actual rules soon. We should try and get that right so that we don't have to reformat again.

@joan38
Copy link
Contributor Author

joan38 commented May 11, 2018

Indeed the rules need to be right from the beginning. Let me know what looks best for you.
Thanks @debasishg and @ijuma for reviewing.

@joan38
Copy link
Contributor Author

joan38 commented May 15, 2018

@ijuma have you been able to play with the settings and see what's best?

@joan38
Copy link
Contributor Author

joan38 commented May 21, 2018

@ijuma @guozhangwang @debasishg do you guys have some time to review this? I'm worried this is going stall.

@ijuma
Copy link
Contributor

ijuma commented May 22, 2018

Sorry, it's the KIP freeze for the next release tomorrow and everyone is busy reviewing KIPs at the moment.

@joan38
Copy link
Contributor Author

joan38 commented May 22, 2018

Make sense let's have a look later then.

@ijuma ijuma added this to the 2.0.0 milestone Jun 9, 2018
@joan38
Copy link
Contributor Author

joan38 commented Jul 4, 2018

@ijuma @guozhangwang @debasishg should we move forward with this?

guozhangwang
guozhangwang reviewed Jul 6, 2018
View reviewed changes
Copy link
Contributor

@guozhangwang guozhangwang left a comment

Choose a reason for hiding this comment

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

Except one minor question, other rules lgtm.

Could you rebase?

checkstyle/.scalafmt.conf
maxColumn = 120
continuationIndent.defnSite = 2
assumeStandardLibraryStripMargin = true
danglingParentheses = true
Copy link
Contributor

Choose a reason for hiding this comment

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

danglingParentheses default seems to be true already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is since 1.6.0 but here we are stuck on 1.5.1 so I specified it explicitly.
This PR diffplug/spotless#260 is the fix for us to be able to upgrade to Scalafmt 1.6.x

guozhangwang
guozhangwang approved these changes Jul 9, 2018
View reviewed changes
Copy link
Contributor

@guozhangwang guozhangwang left a comment

Choose a reason for hiding this comment

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

LGTM.

I would merge this PR to get Streams to follow these rules, and @ijuma can take another pass and see if we'd like to propagate to other modules.

@guozhangwang guozhangwang merged commit 05c5854 into apache:trunk Jul 9, 2018
guozhangwang pushed a commit that referenced this pull request Aug 7, 2018
@joan38 @guozhangwang
MINOR: Add Scalafmt to Streams Scala API (#4965)
ecb7943 
Reviewers: Guozhang Wang <[email protected]>
@guozhangwang
Copy link
Contributor

Cherry-picked to 2.0 as well.

@joan38 joan38 deleted the scalafmt branch August 8, 2018 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guozhangwang guozhangwang guozhangwang approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@joan38 @debasishg @ijuma @guozhangwang