Add @DeprecatedFeature annotation. #187

cmnbroad · 2022-10-04T16:31:36Z

This also fixes an issue where positional args were previously not showing up in command line help.

…sage.

Updates.

codecov · 2022-10-04T16:32:38Z

Codecov Report

Base: 76.66% // Head: 76.69% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (89b9f4c) compared to base (7fe37aa).
Patch coverage: 91.75% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #187      +/-   ##
============================================
+ Coverage     76.66%   76.69%   +0.02%     
- Complexity      744      762      +18     
============================================
  Files            32       33       +1     
  Lines          2576     2609      +33     
  Branches        498      505       +7     
============================================
+ Hits           1975     2001      +26     
- Misses          406      409       +3     
- Partials        195      199       +4

Impacted Files	Coverage Δ
...adinstitute/barclay/help/TemplateMapConstants.java	`0.00% <0.00%> (ø)`
...roadinstitute/barclay/help/TemplateProperties.java	`0.00% <ø> (ø)`
...a/org/broadinstitute/barclay/help/DocWorkUnit.java	`85.71% <66.66%> (-3.65%)`	⬇️
...arclay/argparser/PositionalArgumentDefinition.java	`74.32% <85.00%> (+3.95%)`	⬆️
...nstitute/barclay/argparser/ArgumentDefinition.java	`74.61% <93.33%> (+2.44%)`	⬆️
...e/barclay/argparser/CommandLineArgumentParser.java	`89.44% <100.00%> (+0.13%)`	⬆️
...ute/barclay/argparser/NamedArgumentDefinition.java	`77.81% <100.00%> (-0.78%)`	⬇️
...titute/barclay/help/DefaultDocWorkUnitHandler.java	`73.66% <100.00%> (-0.46%)`	⬇️
.../org/broadinstitute/barclay/help/GSONArgument.java	`100.00% <100.00%> (ø)`
.../org/broadinstitute/barclay/help/GSONWorkUnit.java	`100.00% <100.00%> (ø)`
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

cmnbroad · 2022-10-04T21:50:05Z

@droazen I can't recall any reason why this was left as draft. I think its ready for review.

lbergelson

@cmnbroad A few minor comments.

lbergelson · 2022-10-17T21:45:02Z

src/main/java/org/broadinstitute/barclay/argparser/PositionalArgumentDefinition.java

+    public String getArgumentUsage(final int argumentColumnWidth, final int descriptionColumnWidth) {
+
+        final StringBuilder sb = new StringBuilder();
+        sb.append("--").append("POSITIONAL");


Should this include which position it should be in? Is that easy to find? I

Not sure how to convey it, but positional args must always be first. But these are hardly ever used.

I updated this String to say "POSITIONAL (must be first)".

lbergelson · 2022-10-17T21:46:00Z

src/main/java/org/broadinstitute/barclay/argparser/NamedArgumentDefinition.java

-     * return true if this argument has the {@code @Deprecated} annotation.
-     * @return true if this argument is advanced, otherwise false.
-     */
-    public boolean isDeprecated() { return getUnderlyingField().isAnnotationPresent(Deprecated.class); }


So previously this would be triggered by the standard java Deprecated annotation? Should we continue to respect those as well? I don't remember why exactly we decided to use a separate annotation instead of the default one. So that intellij doesn't tag uses of it?

The old code was carried over from the GATK3 implementation, but never fully embraced, and only worked for args IIRC (not tools or other documented features). I think we used a custom annotation both for the IntelliJ issue, and also because we wanted to be able to specify a reason/detail string, which the Java annotation doesn't have. There are a couple of usages of the old tag in GATK for arguments, but I'd prefer to update them rather than carry around (and test) both annotations.

lbergelson · 2022-10-17T21:46:38Z

src/main/java/org/broadinstitute/barclay/argparser/PositionalArgumentDefinition.java

+        Utils.printSpaces(sb, numSpaces);
+
+        String description = getArgumentDescription();
+        if (isDeprecated()) {


Should this little decorator be factored out so it isn't repeated? Might make it more consistent if we change things in the future.

Done - factored out this decorator plus another method to format the description since that was also duplicated on two places.

lbergelson · 2022-10-17T21:49:03Z

src/main/java/org/broadinstitute/barclay/help/GSONArgument.java

-        this.options = options;
+    @SuppressWarnings("unchecked")
+    public GSONArgument(final Map<String, Object> argMap) {
+        this.summary = argMap.get("summary").toString();


Moving this here seems like an improvement.

lbergelson

@cmnbroad Looks good, one nitpick abut the positionals still, feel free to merge and we cacn always address that later

lbergelson · 2022-11-15T16:48:12Z

src/main/java/org/broadinstitute/barclay/argparser/PositionalArgumentDefinition.java

+    public String getArgumentUsage(final int argumentColumnWidth, final int descriptionColumnWidth) {
+
+        final StringBuilder sb = new StringBuilder();
+        sb.append("--").append("POSITIONAL (must be first)");


I hate to be a pain about this, what happens if there are multiple positional arguments?

@lbergelson There can only be one "positional" option per CLP. It can be a collection, so it can have multiple values, but they have to be first in the list. Its basically just an arg without a name. We should probably just get rid of it, but GATK and Picard each have one, I think.

Huh, I thought we supported multiple positionals, sorry, ignore my comment then.

cmnbroad · 2022-11-22T14:36:53Z

Sorry @lbergelson, I had to add one more commit based on testing with GATK. I messed up the command line display formatting when I factored out display code in this PR, which shows up spectacularly in GATK. We were outputting this:

instead of the output produced with this last commit:

lbergelson · 2022-11-22T16:18:06Z

@cmnbroad Ugh, good catch. Formatting things is hard.

lbergelson · 2022-11-22T16:18:59Z

@cmnbroad Good to merge.

cmnbroad · 2022-11-22T16:50:15Z

Thanks @lbergelson!

cmnbroad added 4 commits July 18, 2022 15:58

DeprecatedFeature tag, support deprecated arguments, fix Positional u…

b662673

…sage.

Make CLP annotations mututally exclusive.

d130ca7

Updates.

Add a doc test for a deprecated CLP.

d90bea3

Clean up.

f47449f

cmnbroad changed the title ~~Add @Deprecated annotation.~~ Add @DeprecatedFeature annotation. Oct 4, 2022

cmnbroad requested a review from droazen October 4, 2022 21:50

lbergelson requested changes Oct 17, 2022

View reviewed changes

cmnbroad added 2 commits November 14, 2022 17:13

Reduce code redundancy.

88b2891

Specify that positional args must be first.

68eec00

lbergelson approved these changes Nov 21, 2022

View reviewed changes

cmnbroad force-pushed the cn_deprecation_tag branch from 592caf7 to 89b9f4c Compare November 22, 2022 14:28

Fix bad formatting introduced in refactoring.

89b9f4c

lbergelson approved these changes Nov 22, 2022

View reviewed changes

cmnbroad merged commit bc47857 into master Nov 22, 2022

cmnbroad deleted the cn_deprecation_tag branch November 22, 2022 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add @DeprecatedFeature annotation. #187

Add @DeprecatedFeature annotation. #187

cmnbroad commented Oct 4, 2022

codecov bot commented Oct 4, 2022 •

edited

Loading

cmnbroad commented Oct 4, 2022

lbergelson left a comment

lbergelson Oct 17, 2022

cmnbroad Nov 14, 2022

cmnbroad Nov 14, 2022

lbergelson Oct 17, 2022

cmnbroad Oct 18, 2022

lbergelson Oct 17, 2022

cmnbroad Nov 14, 2022

lbergelson Oct 17, 2022

lbergelson left a comment

lbergelson Nov 15, 2022

cmnbroad Nov 21, 2022

lbergelson Nov 22, 2022

cmnbroad commented Nov 22, 2022

lbergelson commented Nov 22, 2022

lbergelson commented Nov 22, 2022

cmnbroad commented Nov 22, 2022

Add @DeprecatedFeature annotation. #187

Add @DeprecatedFeature annotation. #187

Conversation

cmnbroad commented Oct 4, 2022

codecov bot commented Oct 4, 2022 • edited Loading

Codecov Report

cmnbroad commented Oct 4, 2022

lbergelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad commented Nov 22, 2022

lbergelson commented Nov 22, 2022

lbergelson commented Nov 22, 2022

cmnbroad commented Nov 22, 2022

codecov bot commented Oct 4, 2022 •

edited

Loading