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

Implement scalafmt #2546

Closed
wants to merge 2 commits into from
Closed

Implement scalafmt #2546

wants to merge 2 commits into from

Conversation

easel
Copy link
Contributor

@easel easel commented Sep 30, 2018

Resolves #1206 and replaces #1731

@codecov-io
Copy link

codecov-io commented Sep 30, 2018

Codecov Report

Merging #2546 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2546   +/-   ##
=======================================
  Coverage   95.17%   95.17%           
=======================================
  Files         359      359           
  Lines        6555     6555           
  Branches      273      273           
=======================================
  Hits         6239     6239           
  Misses        316      316
Impacted Files Coverage Δ
js/src/main/scala/cats/js/instances/future.scala 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2976807...8263693. Read the comment docs.

@easel
Copy link
Contributor Author

easel commented Oct 1, 2018

I don't think the codecov/patch failure is relevant. There's perhaps some additions to the CONTRIBUTING.md or something that should go along with this. Any thoughts?

@@ -0,0 +1 @@
// default settings
Copy link
Member

Choose a reason for hiding this comment

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

Okay, someone has to start the bikeshedding, so here goes:

I'd like to see

align=none
maxColumn = 120

The rest of the default settings I think are good :)

@easel
Copy link
Contributor Author

easel commented Oct 1, 2018

I've pushed another commit applying @LukaJCB's bikeshed suggestions. Both seem reasonable. I like alignment, but for a larger project it will create diff noise that may be less than desirable. 120 columns is a reasonable max in the modern world, although I'd say the "style guide" should recommend 80.

@ceedubs
Copy link
Contributor

ceedubs commented Oct 3, 2018

Awesome, @easel! Thanks a bunch.

This looks good to me, but... how is it such a small changeset? I would expect it to touch a ton of lines. Is it actually running on all of the modules?

@ceedubs
Copy link
Contributor

ceedubs commented Oct 3, 2018

The only changes are in the jvm/js projects which makes me think that maybe our JVM/JS cross-building setup is throwing off scalafmt.

@easel
Copy link
Contributor Author

easel commented Oct 3, 2018

@ceedubs Thats an interesting possibility, the cats cross build stuff is definitely beyond the basics. From my perspective if we can bikeshed out the settings it would be best to just get this merged and then work on applying it to any missing submodules as we can. That way the settings are documented and people can use them on their own.

Copy link
Contributor

@ceedubs ceedubs left a comment

Choose a reason for hiding this comment

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

As I mentioned in a comment, I think that the JVM/JS cross-building is causing most modules to not be picked up, but this still seems like strictly an improvement, so 👍 from me.

@@ -0,0 +1,3 @@
// default settings
Copy link
Contributor

@joan38 joan38 Oct 17, 2018

Choose a reason for hiding this comment

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

Why this comment // default settings ?

@kailuowang
Copy link
Contributor

kailuowang commented Oct 18, 2018

shall we close this since #2562 is merged?

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.

6 participants