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

Switch to Scalafmt #26356

Merged
merged 5 commits into from Mar 11, 2019

Conversation

Projects
None yet
8 participants
@patriknw
Copy link
Member

commented Feb 9, 2019

  • and replace unicode arrows

@akka-ci akka-ci added validating tested and removed validating labels Feb 9, 2019

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Feb 9, 2019

Test PASSed.

@patriknw patriknw force-pushed the wip-scalafmt-patriknw branch from f18b78d to 96ffa51 Feb 11, 2019

@akka-ci akka-ci added validating and removed tested labels Feb 11, 2019

@patriknw

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

One part is missing here, it should also scalafmtOnCompile for multi-jvm sources. Probably some kind of dependsOn.

@akka-ci akka-ci added needs-attention and removed validating labels Feb 11, 2019

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Feb 11, 2019

Test FAILed.

@patriknw patriknw force-pushed the wip-scalafmt-patriknw branch from 96ffa51 to d36d352 Feb 11, 2019

@akka-ci akka-ci added validating and removed needs-attention labels Feb 11, 2019

@patriknw

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

The performance seems ok since it does a good job at format incrementally.

a full scalafmt takes ~50 seconds

If it turns out to be too slow we can disable format on compile, which is what is recommended in docs https://scalameta.org/scalafmt/docs/installation.html#format-on-compile.

Format on save in IntelliJ is probably good to enable, anyway: https://scalameta.org/scalafmt/docs/installation.html#format-on-save

@akka-ci akka-ci added tested and removed validating labels Feb 11, 2019

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Feb 11, 2019

Test PASSed.

@2m

2m approved these changes Feb 11, 2019

Copy link
Member

left a comment

LGTM.

@2m

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

There has recently been improvements to sbt-scalafmt's formatting of code in non-standard configurations and also improved cached formatting:

scalameta/sbt-scalafmt#14
scalameta/sbt-scalafmt#13

Improvements not yet released, tho.

@helena helena self-requested a review Feb 11, 2019

@2m

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

This should enable the formatting for MutiJvm configuration:

settings(
   inConfig(MultiJvm)(scalafmtConfigSettings)
   ...
)
docstrings = JavaDoc
indentOperator = spray
maxColumn = 120
rewrite.rules = [RedundantParens, SortImports]

This comment has been minimized.

Copy link
@helena

helena Feb 11, 2019

Member

You seem to prefer AvoidInfix as well, but we may not want to rewrite those.

This comment has been minimized.

Copy link
@helena

helena Feb 11, 2019

Member

My strong pref is for RedundantBraces as well but...

This comment has been minimized.

Copy link
@patriknw

patriknw Feb 12, 2019

Author Member

RedundantBraces

I think that depends how large the statement is. If it's a real one-liner then yes, but if it's wrapping over several lines and having comments I prefer some extra { } to clarify the scope.

This comment has been minimized.

Copy link
@patriknw

patriknw Feb 12, 2019

Author Member

I would like to use AvoidInfix but it is not nice for some things, like

withAttributes(traversalBuilder.attributes.and(attr))

but maybe the total result is better? I have pushed that as a separate commit so we can compare and decide: 04ae484

This comment has been minimized.

Copy link
@patriknw

patriknw Feb 12, 2019

Author Member

There were some more trouble with AvoidInfix that I have solved in 32199ce and 1627f14

This comment has been minimized.

Copy link
@helena

helena Feb 14, 2019

Member

AvoidInfix seems good now, WDYT?

This comment has been minimized.

Copy link
@patriknw

patriknw Feb 14, 2019

Author Member

rewrite.neverInfix.excludeFilters worked great, now it's AvoidInfix with some listed exceptions

This comment has been minimized.

Copy link
@dwijnand

dwijnand Feb 18, 2019

Member

"some" :D

style = defaultWithAlign

align = some
align.tokens = [off]

This comment has been minimized.

Copy link
@helena

helena Feb 11, 2019

Member

align.tokens = [{code = "=>", owner = "Case"}] might be nice

This comment has been minimized.

Copy link
@patriknw

patriknw Feb 12, 2019

Author Member

thanks, I was looking for that and thought it would be covered by align = some.
added in b042e06

@helena

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

Looks good, just a few suggestions to consider, or not.

@helena

helena approved these changes Feb 11, 2019

Copy link
Member

left a comment

Added just a few things to consider first, otherwise LGTM

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Mar 11, 2019

Test FAILed.

@patriknw

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

RoutersSpec

@patriknw

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

PLS BUILD

@akka-ci akka-ci added validating and removed needs-attention labels Mar 11, 2019

@johanandren

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

@patriknw that RoutersSpec has been failing all weekend I have fixed it in a PR today so maybe just go ahead and merge anyways?

@patriknw

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

I will not merge this (or anything) without green PR validation.

@johanandren

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

In that case, could be good to merge #26497 which fixes the RoutersSpec

@akka-ci akka-ci added tested and removed validating labels Mar 11, 2019

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Mar 11, 2019

Test PASSed.

patriknw and others added some commits Feb 9, 2019

Switch to Scalafmt in build
* AvoidInfix in scalafmt.conf

* align.tokens in scalafmt.conf

* scalafmt in MultiJvm

* sbt-scalafmt 2.0.0-RC5

* scalafmt in all* command alias

* scalafmtAll in CONTRIBUTING.md
replace unicode arrows
* ⇒, →, ←
* because we don't want to show them in documentation snippets and
  then it's complicated to avoid that when snippets are
  located in src/test/scala in individual modules
* dont replace object `→` in FSM.scala and PersistentFSM.scala
manual adjustments before scalafmt
* fix formatting error

* reversePrepend instead of reverse_:::
  * because it is causing trouble for AvoidInfix formatting

* prepare for AvoidInfix

* fix try-catch

@patriknw patriknw force-pushed the wip-scalafmt-patriknw branch from 05098ea to eb5dd7d Mar 11, 2019

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Mar 11, 2019

Test PASSed.

@patriknw patriknw merged commit 4729a80 into master Mar 11, 2019

4 checks passed

Jenkins PR Validation Test PASSed. 8252 tests run, 500 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@patriknw patriknw deleted the wip-scalafmt-patriknw branch Mar 11, 2019

@patriknw patriknw added this to the 2.5.22 milestone Mar 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.