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

Add scalafmt #1731

Closed
wants to merge 1 commit into from
Closed

Add scalafmt #1731

wants to merge 1 commit into from

Conversation

ceedubs
Copy link
Contributor

@ceedubs ceedubs commented Jun 16, 2017

Resolves #1206

A few notes:

  • Currently this is using sbt-scalafmt-coursier, which is a project
    that's in beta status but seems to be working.
  • Currently this is set up so that no code is automatically formatted
    (you have to run the scalafmt command), but if there is a formatting
    issue, the validateJVM command will fail.
  • There weren't that many formatting changes required, which probably
    means some combination of Cats having consistent style and the default
    formatting rules being fairly lenient.

Resolves typelevel#1206

A few notes:
- Currently this is using sbt-scalafmt-coursier, which is a project
that's in beta status but seems to be working.
- Currently this is set up so that no code is automatically formatted
(you have to run the `scalafmt` command), but if there is a formatting
issue, the `validateJVM` command will fail.
- There weren't that many formatting changes required, which probably
means some combination of Cats having consistent style and the default
formatting rules being fairly lenient.
@ceedubs
Copy link
Contributor Author

ceedubs commented Jun 16, 2017

Other things to consider:

  • There may be rules that are useful that aren't enabled by default. I haven't really looked into this.
  • I think that there's still benefit in having scalastyle for some of its rules, but there may be some that are made redundant with scalafmt. This probably is fine, though.

@kailuowang
Copy link
Contributor

👍. I also vote for turning automatical formatting on. I have auto format enabled for all of my projects and never encountered a problem.

@djspiewak
Copy link
Member

I'm generally in favor of enabling auto-formatting, but only after we bikeshed the settings just a little bit. Some of the changes in the diff make me twitch violently, so I'd like the chance to at least make my case as to why it's a bad rule before we universally enforce it.

@mpilquist
Copy link
Member

👍

@ceedubs
Copy link
Contributor Author

ceedubs commented Jun 20, 2017

@djspiewak I just used the default scalafmt configuration because personally I value consistency but am fairly ambivalent on the details. I imagine that a lot of projects have(and will) adopted the default scalafmt settings, so I see a bit of benefit in using them, but I'm also completely fine with an alternative configuration being used. Feel free to submit a PR to my branch or to open a separate PR if you will get to this soon. If you don't see yourself making a PR in the near future, then it may be best for us to merge this for the sake of consistency and we can always make changes in the future (but formatting changes tend to cause merge conflicts).

@djspiewak
Copy link
Member

@ceedubs I expect to loop back 'round to this by Saturday. If you don't have a PR from me by then, consider it an implicit 👍 to merge as-is.

@ceedubs
Copy link
Contributor Author

ceedubs commented Jun 21, 2017

@djspiewak I don't think that merging as-is is an option anyway. There are build errors in scala 2.10 and 2.11. I'm not sure that I completely understand them. I seem to have a recollection of scalafmt not working in scala 2.10? Guidance appreciated 😬

@kailuowang
Copy link
Contributor

my theory is that scalafmt is built on scalameta which doesn't support scala 2.10

@ceedubs ceedubs mentioned this pull request Sep 10, 2018
@easel
Copy link
Contributor

easel commented Sep 25, 2018

I'd love to see this merged. Given the small amount of reformatting that introducing scalafmt with the default settings added out of the gate, I vote to dispense with the bike shedding of settings and add a .scalafmt file referencing the default settings.

@easel
Copy link
Contributor

easel commented Sep 25, 2018

@ceedubs You should have a PR from me with this branch rebased and updated to the latest scalafmt. I'm reasonably hopeful it will "just work" now, it's evolved a bunch since mid 2017. The updated branch is at https://github.com/easel/cats/tree/scalafmt.

@ceedubs
Copy link
Contributor Author

ceedubs commented Sep 30, 2018

Closing out as this branch is stale and @easel looks to be picking up this effort!

@ceedubs ceedubs closed this Sep 30, 2018
@easel
Copy link
Contributor

easel commented Sep 30, 2018

Yeah can do, just didn't want to create a duplicate PR while yours was open. Thanks

@easel easel mentioned this pull request Sep 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants