-
Notifications
You must be signed in to change notification settings - Fork 16
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
Repace Scalastyle with scalafmt #74
Conversation
@carlosms do you think we should add information about scalafmt/plugin in CONTRIBUTING.md? |
``` | ||
make format-scala | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we
- add details on how to configure IDE plugin with the same set of formater/liner ruler here as well
- make sure that after those steps re-formatting between CLI and IDE does not happen
build.sbt
Outdated
@@ -38,13 +37,19 @@ assemblyMergeStrategy in assembly := { | |||
} | |||
|
|||
assemblyShadeRules in assembly := Seq( | |||
ShadeRule.rename("com.google.common.**" -> "com.google.shadedcommon.@1").inAll, | |||
ShadeRule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes like this (and all below) hurt readability, are not useful and shall not be enforced by our formatter
testOptions in Test += Tests.Argument(TestFrameworks.ScalaTest, "-l", "Cassandra") | ||
testOptions in Test += Tests.Argument( | ||
TestFrameworks.ScalaTest, | ||
"-l", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above - this is not usefull
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bzz from my understanding scalafmt tries to do the same what prettier did for javascript. Yes, it doesn't always make useful changes but consistent.
They have very good explanation about it in "why" section of readme: https://prettier.io/docs/en/why-prettier.html
I personally 100% in to sacrifice readability in exchange to consistent code style. (although about readability some people can disagree with you)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consistency is 👍 but autoformatter shall adhere the language styleguide and make things easier to read and maintain, not harder.
If we get back to discussing particular case at hand: following a common sense and the styleguide http://docs.scala-lang.org/style/indentation.html#methods-with-numerous-arguments it seems to me that this change should not happen.
Or did I miss something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it's the case here. But most probably it is. The line is more than 80 character in length. Most probably it's a reason to break arguments to multiple lines. Prettier
and elm-fmt
work like that.
In short, formatter can't know how to break a long line in the most readable way (it is subjectively). So it just puts each argument on new line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS.
easier to read and maintain
is also subjectively. I remember a huge holywar about new elements on new line in a list.
Something like:
a = [1,2]
vs
a = [
1,
2,
]
I think the first variant is better, but surprisingly most of people in the issue voted for the second one.
def findAllDuplicateBlobHashes( | ||
conn: Session, | ||
keyspace: String | ||
): Iterable[DuplicateBlobHash] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes like this, that make concise code much longer, shall not be done by a formatter
Readme updated with IntelliJ instructions. About the style, I couldn't find any other scalafmt.conf widely used by the community, like airbnb's style guide for JS, or pep8 for python. |
@carlosms thank you! Seems like a rebase/reformat might be in order. Could you please verify that this autoformater respects the convention from http://docs.scala-lang.org/style/ ? I.e for http://docs.scala-lang.org/style/indentation.html#methods-with-numerous-arguments
that indentation is not applied in method call, but only for >5 arguments, etc. |
Signed-off-by: Carlos Martín <[email protected]>
Signed-off-by: Carlos Martín <[email protected]>
Signed-off-by: Carlos Martín <[email protected]>
Branch is now rebased. About wrapping method invocation onto multiple lines, as you can see here the arguments are formatted to one per line when the line is too long, but here (Gemini.scala:79) the method is left in one line because it fits. |
@carlosms could you please brake this great work down to two new PRs:
As soon as autoformat follows acceptable coding conventions from http://docs.scala-lang.org/style/ we will re-format the code. |
Awesome, thank you! Closing in favor of 2 smaller PRs. |
Fixes #50 & #49.
The main commit is 16cd1d1. In it scalastyle is removed, and the neo-sbt-scalafmt sbt plugin is added. There are 2 new commands to check and fix formatting,
make lint-scala
andmake format-scala
. Travis will use this new lint.There is an official plugin for IntelliJ, and it can be configured to format code con save.
In 4216c9a the current code is automatically formatted with scalafmt.