Add suppressFileExpansion property to @Argument. #125
Conversation
Codecov Report
@@ Coverage Diff @@
## master #125 +/- ##
============================================
+ Coverage 76% 76.07% +0.06%
- Complexity 591 593 +2
============================================
Files 22 22
Lines 2205 2215 +10
Branches 456 457 +1
============================================
+ Hits 1676 1685 +9
Misses 352 352
- Partials 177 178 +1
Continue to review full report at Codecov.
|
| * Extensions recognized for collection argument file expansion. | ||
| */ | ||
| static final String COLLECTION_LIST_FILE_EXTENSIONS_LIST = ".list"; | ||
| static final String COLLECTION_LIST_FILE_EXTENSIONS_ARGS = ".args"; |
There was a problem hiding this comment.
I think that it might be better to add the class for configuration at this stage (see #95), because each of this changes is generating a breaking change. I think that the configuration is a breaking change but flexible enough to avoid the change of the collection file extension here and keep it only in GATK.
I will open a new PR with that solution to have a clean branch instead of an outdated PR that get lost because its purpose was changed.
There was a problem hiding this comment.
Taking #95 would require a major version bump (by semantic versioning, which I'd like to follow as much as possible), which we just did, and which I'd like to avoid until we can bundle in all other changes that are not BWC. This PR introduces only a minor change in runtime behavior (no source or binary incompatibility), and the runtime behavior change has a pretty easy workaround (to change the file extension being used).
Also, I think we've accumulated enough other feature requests that involve customization that we need an even more general mechanism than the config class as in #95. Instead, I think we should take this change as is, and implement the scheme outlined in (#127) is more sustainable in the long run.
| else { | ||
| expandedValues.add(stringValue); | ||
| private List<String> expandListFile(final ArgumentDefinition argDef, final List<String> originalValues) { | ||
| if (!argDef.suppressFileExpansion) { |
There was a problem hiding this comment.
I like this change (although I prefer to being able to change the extension in a per-argument basis), but the configuration sounds much better for the change of the extensions itself. Have a look to the clean PR (I will comment with the number once it is here).
There was a problem hiding this comment.
New PR for config interface in #126
| * Expand any collection value that references a filename that ends in one of the accepted expansion | ||
| * extensions, and add the contents of the file to the list of values for that argument. | ||
| * @param argDef the ArgumentDefinition being populated | ||
| * @param originalValues |
There was a problem hiding this comment.
Add explanation of parameter
|
@cmnbroad This looks fine to me. One very minor comment. |
…tion to .args as a file expansion extension, and handle display representation of long command lines.
cmnbroad
force-pushed
the
cn_file_expansion
branch
from
6f419fa to
082e906
Compare
accept .list in addition to .args as a file expansion extension, and handle display representation of long command lines.