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

Add DeprecatedFeature annotation. #8100

Merged
merged 2 commits into from
Dec 19, 2022
Merged

Add DeprecatedFeature annotation. #8100

merged 2 commits into from
Dec 19, 2022

Conversation

cmnbroad
Copy link
Collaborator

@cmnbroad cmnbroad commented Nov 21, 2022

Upgrades Barclay to 4.1.0. Adds an @DeprecatedFeature annotation that can be applied to any @DocumentedFeature or @Argument, and updates the handful of such features that are already tagged with the standard Java annotation @Deprecated to use the new annotation. Mutually exclusive with @Beta and @Experimental.

The output for --use-new-qual-calculator:
Screen Shot 2022-11-21 at 5 04 44 PM
Screen Shot 2022-11-21 at 5 14 16 PM
Screen Shot 2022-11-21 at 6 39 03 PM

@cmnbroad cmnbroad marked this pull request as draft November 21, 2022 22:13
@cmnbroad cmnbroad marked this pull request as ready for review November 22, 2022 21:39
@gatk-bot
Copy link

Github actions tests reported job failures from actions build 3527272492
Failures in the following jobs:

Test Type JDK Job ID Logs
integration 11 3527272492.12 logs

@codecov
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

Merging #8100 (2916433) into master (ca33bc9) will increase coverage by 2.535%.
The diff coverage is 100.000%.

❗ Current head 2916433 differs from pull request most recent head a428cff. Consider uploading reports for the commit a428cff to get more accurate results

Additional details and impacted files
@@               Coverage Diff               @@
##              master     #8100       +/-   ##
===============================================
+ Coverage     84.069%   86.604%   +2.535%     
- Complexity     37456     38940     +1484     
===============================================
  Files           2335      2335               
  Lines         182679    182680        +1     
  Branches       20052     20052               
===============================================
+ Hits          153577    158209     +4632     
+ Misses         22059     17433     -4626     
+ Partials        7043      7038        -5     
Impacted Files Coverage Δ
...te/hellbender/cmdline/DeprecatedToolsRegistry.java 90.000% <ø> (ø)
...notyper/GenotypeCalculationArgumentCollection.java 89.474% <100.000%> (ø)
...allerReadThreadingAssemblerArgumentCollection.java 62.500% <100.000%> (ø)
...der/tools/walkers/mutect/M2ArgumentCollection.java 95.000% <100.000%> (+21.667%) ⬆️
...titute/hellbender/utils/help/GATKGSONWorkUnit.java 100.000% <100.000%> (ø)
...nstitute/hellbender/utils/help/GATKHelpDoclet.java 100.000% <100.000%> (ø)
...utectReadThreadingAssemblerArgumentCollection.java 57.143% <0.000%> (-7.143%) ⬇️
...institute/hellbender/tools/spark/bwa/BwaSpark.java 83.333% <0.000%> (-5.556%) ⬇️
...k/pipelines/BwaAndMarkDuplicatesPipelineSpark.java 78.947% <0.000%> (-5.263%) ⬇️
...tools/funcotator/metadata/SamplePairExtractor.java 95.000% <0.000%> (-5.000%) ⬇️
... and 172 more

@cmnbroad cmnbroad force-pushed the cn_deprecation_tag branch 2 times, most recently from a428cff to 8940fae Compare November 29, 2022 15:27
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.

@cmnbroad Minor comments.

It might be nice to replace the (Deprecated) mark in the html with something more like the little bubbles for the input type so it stands out a bit more. (even if it's in its own dedicated section.)

203168110-381424c7-e0e2-4f93-b51b-41f74212b669

203168311-cfc7b1b1-79d8-48c8-b965-766e424135ab

@@ -32,7 +33,7 @@ public class HaplotypeCallerReadThreadingAssemblerArgumentCollection extends Rea
/**
* As of version 3.3, this argument is no longer needed because dangling end recovery is now the default behavior. See GATK 3.3 release notes for more details.
*/
@Deprecated
@DeprecatedFeature
Copy link
Member

Choose a reason for hiding this comment

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

This has been deprecated since 3.3... maybe we can remove it.

Copy link
Member

Choose a reason for hiding this comment

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

Can we put the reason in the description?

@@ -115,7 +116,7 @@ public ReadThreadingAssembler createReadThreadingAssembler(){
public File f1r2TarGz;

// As of GATK 4.1, any sample not specified as the normal is considered a tumor sample
@Deprecated
@DeprecatedFeature
Copy link
Member

Choose a reason for hiding this comment

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

Should we move the reason into here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. Done.

@@ -61,7 +62,7 @@ public GenotypeCalculationArgumentCollection clone() {
/**
* As of version 4.1.0.0, this argument is no longer needed because the new qual score is now on by default. See GATK 3.3 release notes for more details.
Copy link
Member

Choose a reason for hiding this comment

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

Does javadoc on arguments get output in our docs at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it gets included in the full text description.

@lbergelson
Copy link
Member

@cmnbroad Are we waiting on something to merge this? 👍 from me.

@cmnbroad cmnbroad merged commit 273bfa9 into master Dec 19, 2022
@cmnbroad cmnbroad deleted the cn_deprecation_tag branch December 19, 2022 19:20
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.

3 participants