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

Do not resolve classes dirs as a file tree #2102

Merged
merged 3 commits into from May 24, 2017
Merged

Conversation

big-guy
Copy link
Member

@big-guy big-guy commented May 22, 2017

This causes us to include resources in a way that causes FindBugs to
produce an annoying error message

Context

Contributor Checklist

  • Review Contribution Guidelines
  • Sign Gradle CLA
  • Link to Design Spec for changes that affect more than 1 public API (that is, not in an internal package) or updates to > 20 files
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
  • Provide unit tests (under <subproject>/src/test) to verify logic
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes
  • Ensure that tests pass locally: ./gradlew quickCheck <impacted-subproject>:check

Gradle Core Team Checklist

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation including proper use of @since and @Incubating annotations for all public APIs
  • Recognize contributor in release notes

This causes us to include resources in a way that causes FindBugs to
produce an annoying error message
@big-guy big-guy self-assigned this May 22, 2017
@big-guy big-guy requested a review from wolfs May 22, 2017 06:24
Copy link
Member

@wolfs wolfs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice touch with only taking class files into account for up to date checks. I a comments regarding naming and javadocs.

@@ -328,12 +329,26 @@ public FileTree getSource() {
@SkipWhenEmpty
@PathSensitive(PathSensitivity.RELATIVE)
@InputFiles
public FileCollection getCandidateClasses() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A javadoc would be nice here.
We have getCandidateClassFiles() on the Test task. Should we use the same name here?
Should we make this method protected so we somewhat hide it for consumers of the class?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The javadoc is hidden in the diff.

But on second thought about this, I think filtering by .class files might be wrong here. I think nothing stops someone from doing something like:

findbugsMain {
   classes = files("someJar.jar")
}

And this would work. I took your advice and renamed the method and made it less visible.

Copy link
Member

@wolfs wolfs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice touch with only taking class files into account for up to date checks. I added a comments regarding naming and javadocs.

Copy link
Member

@wolfs wolfs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment. I would now remove the new method completely.

@@ -328,12 +328,21 @@ public FileTree getSource() {
@SkipWhenEmpty
@PathSensitive(PathSensitivity.RELATIVE)
@InputFiles
protected FileCollection getCandidateClassFiles() {
return getClasses().getAsFileTree();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this now different to annotating getClasses directly? If we do not filter by *.class then I would actually not add this new method but just use the old method as is and only change the convention mapping.

@wolfs
Copy link
Member

wolfs commented May 23, 2017

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants