-
Notifications
You must be signed in to change notification settings - Fork 6
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 the list file argument expansion extension and add a new option #95
Closed
magicDGS
wants to merge
4
commits into
broadinstitute:master
from
bioinformagik:dgs_rename_arg_expansion_file
Closed
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
80e170e
Rename extension for list file argument expansion
magicDGS 51000c2
Add CommandLineParserOptions to allow removal of args file parsing
magicDGS 5ffbf91
Implement CommandLineParserOption abstractioN
magicDGS 5d07ecd
Add CommandLineParserConfig
magicDGS File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9 changes: 9 additions & 0 deletions
9
src/main/java/org/broadinstitute/barclay/argparser/parseropt/CommandLineParserOption.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| package org.broadinstitute.barclay.argparser.parseropt; | ||
|
|
||
| /** | ||
| * Marker interface for options used to control command line parser behavior. | ||
| * | ||
| * @author Daniel Gomez-Sanchez (magicDGS) | ||
| */ | ||
| public interface CommandLineParserOption { | ||
| } |
41 changes: 41 additions & 0 deletions
41
.../java/org/broadinstitute/barclay/argparser/parseropt/ExpandCollectionExtensionOption.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| package org.broadinstitute.barclay.argparser.parseropt; | ||
|
|
||
| import org.broadinstitute.barclay.utils.Utils; | ||
|
|
||
| import java.util.Collection; | ||
| import java.util.Collections; | ||
|
|
||
| /** | ||
| * Option for loading argument collections from files ending with concrete extensions. | ||
| * | ||
| * @author Daniel Gomez-Sanchez (magicDGS) | ||
| */ | ||
| public final class ExpandCollectionExtensionOption implements CommandLineParserOption { | ||
|
|
||
| /** | ||
| * Default xtension for collection argument list files. | ||
| */ | ||
| public static final String DEFAULT_COLLECTION_LIST_FILE_EXTENSION = ".args"; | ||
|
|
||
| private final Collection<String> extensions; | ||
|
|
||
| /** Constructor for a collection of extensions. */ | ||
| public ExpandCollectionExtensionOption(final Collection<String> extensions) { | ||
| this.extensions = Utils.nonNull(extensions); | ||
| } | ||
|
|
||
| /** | ||
| * Default constructor uses {@link #DEFAULT_COLLECTION_LIST_FILE_EXTENSION}. | ||
| */ | ||
| public ExpandCollectionExtensionOption() { | ||
| this(Collections.singleton(DEFAULT_COLLECTION_LIST_FILE_EXTENSION)); | ||
| } | ||
|
|
||
| /** | ||
| * Load options for argument collections from files if the argument value ends with one of the | ||
| * provided extensions. | ||
| */ | ||
| public Collection<String> getExtensions() { | ||
| return extensions; | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we now want to have more complex options for things like custom expansion file extensions, I think it would be cleaner to switch over to using a configuration object that is passed in by clients. Barclay can have a default implementation, with methods like "List getExpansionFileExtensions()", and custom implementations be provided by the clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So do you mean to use a
BarclayConfinterface (or mutable object) to pass to the parser? I think that passing the options as single elements makes clear which behaviour is changing/requested...If we are going to that direction, I can either make another commit here or open a new PR, whatever you prefer...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that instead of the CommandLineParserOption enum or interface, Barclay should have a CommandLineParserConfig class, with methods like "List getExpansionFileExtensions()" and "boolean getAppendToCollections()", with default implementations. Any customizations would be implemented by passing a config object to the constructor which is a subclass of that default implementation. Barclay would just query it; hopefully the object itself won't need any mutators.
When we just had options that were toggles, it would have been overkill, but with the need to return a custom list, this is cleaner, and will eliminate the need for type casting, etc. Reusing this PR is fine, or make a new one if its easier - either way. Thanks!