scalastyle / scalastyle-sbt-plugin

scalastyle-sbt-plugin
Apache License 2.0
139 stars 52 forks source link

Read sources files from sbt instead of scalaSource #41

Closed adrian-wang closed 9 years ago

adrian-wang commented 9 years ago

This is to enable us to run scalastyle with sources that are dynamically added when we are using sbt. (for example, we can use sbt-maven-plugin and build-helper-maven-plugin to add some additional source file, check https://github.com/apache/spark/blob/master/sql/hive-thriftserver/pom.xml )

Originally, scalastyle-sbt-plugin would call scalaSource of sbt command to get all the files that need to be checked, but this would omit those file that not in src/main/scala/{groupId}/xxx.

In this patch, we are calling sources of sbt command, and get all the files that the sbt would compile.

/cc @matthewfarwell This is a solution to https://issues.apache.org/jira/browse/SPARK-4331

matthewfarwell commented 9 years ago

The change you've made would include both managed and unmanaged sources in the scalastyle analysis. If I understand correctly, this is reasonable in your case. I don't think this should be the default behaviour, though. Generated sources often have lots of style issues. Also, is there any reason why you're using the task (sources) rather than the setting (sourceDirectories)?

I would suggest that we add a scalastyleSources setting with default value of Seq(scalaSource), which can then be overridden in the .sbt file to be sourceDirectories?

What do you think?

adrian-wang commented 9 years ago

oh, I didn't know about sourceDirectories. I'll try to apply your suggestion.

Thanks for your time!

matthewfarwell commented 9 years ago

Have you done the change?

ghost commented 9 years ago

+1 As this would also enabled cross-compiled builds to work. For example, https://github.com/non/cats/tree/master/laws is cross compiled to JS and JVM, with shared code in the shared directory. Currently, this is not picked up.

This will also we the case for spire, referenced in https://github.com/scalastyle/scalastyle-sbt-plugin/issues/42 by @rklaehn - eg https://github.com/non/spire/tree/master/core

adrian-wang commented 9 years ago

Hopefully I'll get some time to work on it this weekend.

adrian-wang commented 9 years ago

@matthewfarwell I've done updating this, could you take a look at it?

jkerfs commented 9 years ago

It looks good to me, did you have a chance to look at my pull request for scalastyle (https://github.com/scalastyle/scalastyle/pull/158)?
When we merge that change, we could add a key in the sbt plugin to specify what paths should be excluded from the scalastyle check. That would give us more fine-grained control over which folders in the sourceDirectories should be checked rather than doing only the scalaSource folder or all of the sourceDirectories folders.

adrian-wang commented 9 years ago

@jkerfs Thanks for your comment. I have taken a look at your PR, your solution would benefit the control of which files to be checked. And since the current sbt plugin would only consider scalaSource, we need to expand it first. Maybe after your PR merged, we could change the default value of scalastyleSources here.

ummels commented 9 years ago

+1 I also have a cross-compiled project with sources in the shared directory, which are not picked up.

matthewfarwell commented 9 years ago

Thanks!