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

Change gatkDoc and gatkTabComplete build tasks to include Picard. #3683

Merged
merged 1 commit into from
Oct 16, 2017

Conversation

cmnbroad
Copy link
Collaborator

No description provided.

@cmnbroad
Copy link
Collaborator Author

Fixes #3627.

@codecov-io
Copy link

codecov-io commented Oct 10, 2017

Codecov Report

Merging #3683 into master will increase coverage by 0.007%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##              master     #3683       +/-   ##
===============================================
+ Coverage     79.402%   79.408%   +0.007%     
- Complexity     17369     17370        +1     
===============================================
  Files           1139      1139               
  Lines          62690     62681        -9     
  Branches        9527      9526        -1     
===============================================
- Hits           49777     49774        -3     
+ Misses          9112      9109        -3     
+ Partials        3801      3798        -3
Impacted Files Coverage Δ Complexity Δ
...broadinstitute/hellbender/utils/test/BaseTest.java 83.704% <0%> (-0.12%) 36% <0%> (ø)
...te/hellbender/engine/filters/SampleReadFilter.java 85.714% <0%> (ø) 3% <0%> (ø) ⬇️
...ender/engine/filters/MappingQualityReadFilter.java 100% <0%> (ø) 5% <0%> (ø) ⬇️
.../hellbender/engine/filters/ReadNameReadFilter.java 66.667% <0%> (ø) 3% <0%> (ø) ⬇️
...r/engine/filters/ReadGroupBlackListReadFilter.java 83.333% <0%> (ø) 17% <0%> (ø) ⬇️
...llbender/engine/filters/OverclippedReadFilter.java 100% <0%> (ø) 9% <0%> (ø) ⬇️
...ender/engine/filters/FragmentLengthReadFilter.java 100% <0%> (ø) 4% <0%> (ø) ⬇️
...te/hellbender/engine/filters/ReadStrandFilter.java 66.667% <0%> (ø) 3% <0%> (ø) ⬇️
...e/hellbender/engine/filters/ReadFilterLibrary.java 95.238% <0%> (ø) 1% <0%> (ø) ⬇️
...e/filters/AlignmentAgreesWithHeaderReadFilter.java 100% <0%> (ø) 3% <0%> (ø) ⬇️
... and 5 more

Copy link
Contributor

@magicDGS magicDGS left a comment

Choose a reason for hiding this comment

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

Two suggestions for grabbing the dogen source dependencies not only for Picard...

build.gradle Outdated
@@ -112,7 +113,8 @@ dependencies {
testCompile(javadocJDKFiles)

compile 'org.broadinstitute:barclay:1.2.1'
compile 'com.github.broadinstitute:picard:2.12.2'
compile 'com.github.broadinstitute:picard:' + picardVersion
compileOnly 'com.github.broadinstitute:picard:' + picardVersion + ':sources'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a new configuration for this, to being able to include more in the future:

configurations {
    ## requires the ":sources" marker
    docgen {
        transitive false
    }
}

..

dependencies {
    ..

    docgen 'com.github.broadinstitute:picard:' + picardVersion + ':sources'

    ..
}

build.gradle Outdated
@@ -481,7 +496,7 @@ task gatkDoc(type: Javadoc, dependsOn: classes) {
into gatkDocDir
}
}
source = sourceSets.main.allJava
source = sourceSets.main.allJava + getPicardJavaSourceFiles(picardVersion)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that getPicardJavaSourceFiles can be removed and being simplify the gatkDoc/gatkTabComplete by using:

 final docSources = configurations.docgen.collect { zipTree(it) }
source = files(docSources) + sourceSets.main.allJava
include '**/*.java'

This will also help to include new dependencies in the code and not only Picard....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@magicDGS - thanks - I'll try that. Although I don't imagine we'll add any more source sets like this, your suggestion seems cleaner all around.

@droazen
Copy link
Contributor

droazen commented Oct 13, 2017

@lbergelson You want to have a look at this one before merge?

@cmnbroad
Copy link
Collaborator Author

@droazen Do we want merged javadoc as well (I included that here) ?

build.gradle Outdated
compile.exclude group: 'com.esotericsoftware.kryo'

externalSourceConfiguration {
// External sources we need for doc and tab completion generation tasks )i.e., Picard sources)
Copy link
Member

Choose a reason for hiding this comment

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

typo ) is facing the wrong way

Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

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

One really minor comment 👍 to merge. Gradle stuff seems sane.

@lbergelson lbergelson assigned cmnbroad and unassigned lbergelson Oct 13, 2017
@lbergelson
Copy link
Member

@magicDGS Thanks for that suggestion.

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.

5 participants