Skip to content

Repace Scalastyle with scalafmt #74

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

Closed
carlosms wants to merge 3 commits into from
Closed

Repace Scalastyle with scalafmt #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2e4b927
Repace Scalastyle with scalafmt, add make cmds
carlosms Mar 16, 2018
4b5855d
Add new make cmds to readme
carlosms Mar 20, 2018
01157c6
Apply make format-scala
carlosms Apr 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .scalafmt.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
style = IntelliJ

project.excludeFilters = [
".*com/google/.*"
".*tech/sourced/featurext/generated/.*"
".*gopkg/in/.*"
]
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ matrix:

- scala: 2.11.2
env: STYLE_CHECK=true
script: ./sbt scalastyle
script: make lint-scala

- scala: 2.11.2
env: FE_PYTHON_TEST=true
Expand Down
10 changes: 10 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,13 @@ format-python:

lint-python:
$(YAPF_CMD) --diff

format-scala:
$(SBT) scalafmt
$(SBT) test:scalafmt
$(SBT) sbt:scalafmt

lint-scala:
$(SBT) scalafmt::test
$(SBT) test:scalafmt::test
$(SBT) sbt:scalafmt::test
12 changes: 12 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,18 @@ To generate gRPC code for Feature Extractors from `src/main/proto/*.proto` files
./src/main/resources/generate_from_proto.sh
```

This project uses [scalafmt](http://scalameta.org/scalafmt) formatting. You can configure IntelliJ IDEA to use the same formatting configuration. To do so, follow the [scalafmt instructions](http://scalameta.org/scalafmt/#IntelliJ).

To check if the code follows the project style, use the linters
```
make lint-scala
```

To fix formatting issues, use the formatters
```
make format-scala
```

Copy link
Contributor

@bzz bzz Mar 21, 2018

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

## License

Copyright (C) 2017 source{d}.
Expand Down
53 changes: 27 additions & 26 deletions build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,8 @@ libraryDependencies ++= Seq(
scalaTest % Test,
scoverage % Test,
spark % Test,

sparkSql % Provided,
fixNewerHadoopClient % Provided, //due to newer v. of guava

scalaLib % Compile,
scalapb % Compile,
scalapbGrpc % Compile,
Expand All @@ -29,15 +27,16 @@ libraryDependencies ++= Seq(
log4j12 % Compile,
log4jBinding % Compile
)
assemblyOption in assembly := (assemblyOption in assembly).value.copy(includeScala = false)
assemblyOption in assembly := (assemblyOption in assembly).value
.copy(includeScala = false)
assemblyJarName in assembly := s"${name.value}-uber.jar"

assemblyMergeStrategy in assembly := {
case "META-INF/io.netty.versions.properties" => MergeStrategy.last
// engine uses bblfsh client scala, which also uses scalapb but different version
case PathList("scalapb", xs @ _*) => MergeStrategy.first
case PathList("scalapb", xs @ _*) => MergeStrategy.first
case PathList("com", "trueaccord", "lenses", xs @ _*) => MergeStrategy.first
case PathList("com", "google", "protobuf", xs @ _*) => MergeStrategy.first
case PathList("com", "google", "protobuf", xs @ _*) => MergeStrategy.first
case x =>
val oldStrategy = (assemblyMergeStrategy in assembly).value
oldStrategy(x)
Expand All @@ -49,7 +48,11 @@ assemblyShadeRules in assembly := Seq(

test in assembly := {}
testOptions in Test += Tests.Argument(TestFrameworks.ScalaTest, "-oUT")
testOptions in Test += Tests.Argument(TestFrameworks.ScalaTest, "-l", "Cassandra")
testOptions in Test += Tests.Argument(
TestFrameworks.ScalaTest,
"-l",
Copy link
Contributor

@bzz bzz Mar 21, 2018

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

Copy link
Contributor

@smacker smacker Mar 21, 2018

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)

Copy link
Contributor

@bzz bzz Apr 3, 2018

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?

Copy link
Contributor

@smacker smacker Apr 3, 2018

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.

Copy link
Contributor

@smacker smacker Apr 3, 2018

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.

"Cassandra"
)

parallelExecution in Test := false
fork in Test := true //Forking is required for the Embedded Cassandra
Expand All @@ -59,27 +62,37 @@ sonatypeProfileName := "tech.sourced"

// pom settings for sonatype
homepage := Some(url("https://github.com/src-d/gemini"))
scmInfo := Some(ScmInfo(url("https://github.com/src-d/gemini"),
"git@github.com:src-d/gemini.git"))
developers += Developer("bzz",
scmInfo := Some(
ScmInfo(
url("https://github.com/src-d/gemini"),
"git@github.com:src-d/gemini.git"
)
)
developers += Developer(
"bzz",
"Alexander Bezzubov",
"alex@sourced.tech",
url("https://github.com/bzz"))
url("https://github.com/bzz")
)
licenses += ("GPLv3", url("https://www.gnu.org/licenses/gpl.html"))
pomIncludeRepository := (_ => false)

crossPaths := false
publishMavenStyle := true

val SONATYPE_USERNAME = scala.util.Properties.envOrElse("SONATYPE_USERNAME", "NOT_SET")
val SONATYPE_PASSWORD = scala.util.Properties.envOrElse("SONATYPE_PASSWORD", "NOT_SET")
val SONATYPE_USERNAME =
scala.util.Properties.envOrElse("SONATYPE_USERNAME", "NOT_SET")
val SONATYPE_PASSWORD =
scala.util.Properties.envOrElse("SONATYPE_PASSWORD", "NOT_SET")
credentials += Credentials(
"Sonatype Nexus Repository Manager",
"oss.sonatype.org",
SONATYPE_USERNAME,
SONATYPE_PASSWORD)
SONATYPE_PASSWORD
)

val SONATYPE_PASSPHRASE = scala.util.Properties.envOrElse("SONATYPE_PASSPHRASE", "not set")
val SONATYPE_PASSPHRASE =
scala.util.Properties.envOrElse("SONATYPE_PASSPHRASE", "not set")

useGpg := false
pgpSecretRing := baseDirectory.value / "project" / ".gnupg" / "secring.gpg"
Expand All @@ -96,15 +109,3 @@ publishTo := {
Some("releases" at nexus + "service/local/staging/deploy/maven2")
}
}

scalastyleSources in Compile := {
// all .scala files in "src/main/scala"
val scalaSourceFiles = ((scalaSource in Compile).value ** "*.scala").get
val fSep = java.io.File.separator
val dirsNameToExclude = List(
"com" + fSep + "google",
"tech" + fSep + "sourced" + fSep + "featurext" + fSep + "generated",
"gopkg" + fSep + "in"
)
scalaSourceFiles.filterNot(f => dirsNameToExclude.exists(dir => f.getAbsolutePath.contains(dir)))
}
2 changes: 1 addition & 1 deletion project/plugins.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
addSbtPlugin("com.eed3si9n" % "sbt-assembly" % "0.14.5")
addSbtPlugin("net.virtual-void" % "sbt-dependency-graph" % "0.8.2")
addSbtPlugin("org.scoverage" % "sbt-scoverage" % "1.5.1")
addSbtPlugin("org.scalastyle" %% "scalastyle-sbt-plugin" % "1.0.0")
addSbtPlugin("com.typesafe.sbteclipse" % "sbteclipse-plugin" % "5.2.2")
addSbtPlugin("com.jsuereth" % "sbt-pgp" % "1.0.0")
addSbtPlugin("org.xerial.sbt" % "sbt-sonatype" % "1.1")
addSbtPlugin("com.lucidchart" % "sbt-scalafmt" % "1.15")
98 changes: 0 additions & 98 deletions scalastyle-config.xml
View file
Open in desktop

This file was deleted.

Loading