-
Notifications
You must be signed in to change notification settings - Fork 28
Description
Improvement Description
This came from a discussion I had with @ebolyen on Slack recently and he can fill in technical blanks I'm omitting here.
To re-word the --p-exclude-ids description to clearly describe expected input and behavior.
Current Behavior
Using feature-table filter-samples below but same applies to all 3 filtering actions. Currently, it reads:
--p-exclude-ids / --p-no-exclude-ids
If true, the samples selected by `metadata` or `where`
parameters will be excluded from the filtered table
instead of being retained. [default: False]
It is unclear what inputs are expected i.e. true, True, F etc.
Without an input, it suggests that it would behave as --p-no-exclude-ids defaulting to False...but it doesn't in all cases, see below.
There are 6 potential options for the user here which is too many...
- not setting the parameter at all, defaults to False -> doesn't exclude the ids
- Setting
--p-exclude-ids \with no arg should default to False based on its description, however, it works just like--p-exclude-ids Trueinstead. - Setting
--p-exclude-idswith a True arg -> excludes ids - Setting
--p-exclude-idswith a False arg -> doesn't exclude ids - Setting
--p-no-exclude-idswith a True arg -> doesn't exclude ids - Setting
--p-no-exclude-idswith a False arg ==--p-exclude-ids False🙃
This gets confusing from a user perspective, especially with the double negatives.
Proposed Behavior
Ideally, the user would only have to make a True/False choice, and there wouldn't be a --p-no-exclude-ids at all. Ex.
--p-exclude-ids
If `True`, the samples selected by `metadata` or `where`
parameters will be excluded from the filtered table,
`False` will retain only those samples. [required] [default: True]
Or make it so that --p-exclude-ids and --p-no-exclude-ids take no argument, but one is always required.
Comments
In discussion with Evan, it doesn't sound like there'd be an easy solution like I described above due to the way the parser is set up. Instead, we could just remove the --p-no-exclude-ids option all together from the help files alone and it could just be an easter egg that is there without being shown.