Disable "null..." comment in primitive types

[yfarjoun]

• Removed note "This option can be set to 'null' to clear the default value." for primitive types.
• Added a test to show that it works
• Made sure that there's at least one space between arguments and description

[codecov-io]

Codecov Report

Merging #118 into master will increase coverage by 0.13%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #118      +/-   ##
============================================
+ Coverage     75.69%   75.83%   +0.13%     
- Complexity      580      584       +4     
============================================
  Files            22       22              
  Lines          2181     2181              
  Branches        449      449              
============================================
+ Hits           1651     1654       +3     
  Misses          351      351              
+ Partials        179      176       -3

Impacted Files	Coverage Δ	Complexity Δ
...lay/argparser/LegacyCommandLineArgumentParser.java	75.88% <100%> (+0.62%)	127 <0> (+4)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b14693a...f3d3d34. Read the comment docs.

[magicDGS]

Does this only happen in the legacy parser? I guess that for concordance, it should be modified in the CommandLineArgumentParser

[cmnbroad]

Can you make this change in the other parser as well ?

[magicDGS]

This line does not appear in the CommandLineArgumentParser implementation. Is it better to remove from here too?

[cmnbroad]

The other parser does allow "null" for optional args. There are some tests there for it, but please make the same change to check for isPrimitive and add the same usage test there as well.

[yfarjoun]

No as the two parsers deal differently with the input "null" (which I think is not allowed in the new parser)

…

On Mon, Dec 11, 2017 at 4:24 AM, Daniel Gómez-Sánchez < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/java/org/broadinstitute/barclay/argparser/ LegacyCommandLineArgumentParser.java <#118 (comment)> : > @@ -644,7 +644,7 @@ private void printOptionParamUsage(final StringBuilder sb, final String name, fi } int numSpaces = OPTION_COLUMN_WIDTH - optionLabel.length(); - if (optionLabel.length() > OPTION_COLUMN_WIDTH) { + if (optionLabel.length() > OPTION_COLUMN_WIDTH - 1) { Does this only happen in the legacy parser? I guess that for concordance, it should be modified in the CommandLineArgumentParser ------------------------------ In src/main/java/org/broadinstitute/barclay/argparser/ LegacyCommandLineArgumentParser.java <#118 (comment)> : > @@ -672,7 +672,7 @@ private String makeOptionDescription(final OptionDefinition optionDefinition) { sb.append("Default value: "); sb.append(optionDefinition.defaultValue); sb.append(". "); - if (!optionDefinition.defaultValue.equals("null")) { + if (!optionDefinition.defaultValue.equals("null") && !optionDefinition.field.getType().isPrimitive() ) { This line does not appear in the CommandLineArgumentParser implementation. Is it better to remove from here too? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#118 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACnk0gJhob9d85rtu41lgPBWyRwSjJcyks5s_PS0gaJpZM4Q8XQO> .

[yfarjoun]

@cmnbroad when you have a moment...

[cmnbroad]

The changes seem fine, but please propagate them to the other parser we well. Thanks.