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

Read sources files from sbt instead of scalaSource #41

Merged
merged 3 commits into from
Oct 15, 2015

Conversation

adrian-wang
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

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

Thanks for your time!

@matthewfarwell
Copy link
Member

Have you done the change?

@ghost
Copy link

ghost commented Aug 26, 2015

+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 #42 by @rklaehn - eg https://github.com/non/spire/tree/master/core

@adrian-wang
Copy link
Contributor Author

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

@adrian-wang
Copy link
Contributor Author

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

@jkerfs
Copy link

jkerfs commented Sep 11, 2015

It looks good to me, did you have a chance to look at my pull request for scalastyle (scalastyle/scalastyle#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
Copy link
Contributor Author

@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
Copy link

ummels commented Oct 9, 2015

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

matthewfarwell added a commit that referenced this pull request Oct 15, 2015
Read sources files from sbt instead of scalaSource
@matthewfarwell matthewfarwell merged commit 255e554 into scalastyle:master Oct 15, 2015
@matthewfarwell
Copy link
Member

Thanks!

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.

4 participants