Add support for mutually exclusive arguments #109
Conversation
KubqoA
force-pushed
the
dm/102-refactor-signing-validation
branch
from
6d42857 to
9758b94
Compare
KubqoA
force-pushed
the
dm/106-mutually-exclusive-arguments
branch
from
eb6b8d5 to
d4fd03f
Compare
davidmisiak
force-pushed
the
dm/102-refactor-signing-validation
branch
from
9758b94 to
35fc2a6
Compare
davidmisiak
force-pushed
the
dm/106-mutually-exclusive-arguments
branch
from
d4fd03f to
955aadb
Compare
src/command-parser/commandParser.ts
Outdated
| const initParser = (parser: ArgumentParser | ArgumentGroup, config: any): void => { | ||
| const MUTUALLY_EXCLUSIVE_GROUP_KEY = '_mutually-exclusive-group' | ||
| const isMutuallyExclusiveGroup = (str: string) => str.startsWith(MUTUALLY_EXCLUSIVE_GROUP_KEY) | ||
| const isOneOfGroupRequired = (str: string) => str.endsWith(`${MUTUALLY_EXCLUSIVE_GROUP_KEY}-required`) |
There was a problem hiding this comment.
Shouldn't it be at the start of the string? But we are using endsWith.
There was a problem hiding this comment.
Don't we have some unit tests for command parser? This definitely deserves a test, one for optional, one for required mutually exclusive group (maybe having two groups, too).
There was a problem hiding this comment.
Shouldn't it be at the start of the string? But we are using
endsWith.
Thanks for noticing! That was really a stupid mistake. (I remember testing it manually, but perhaps I forgot to rerun yarn build-js or something.)
Don't we have some unit tests for command parser? This definitely deserves a test, one for optional, one for required mutually exclusive group (maybe having two groups, too).
We do, but they seem more like integration tests for command parsing - the tests are verifying whether the entire input from command line matches expected ParsedArguments after parsing, not whether individual parsers work as expected. I don't think developing an entire new command parser testing approach is worth it (considering that the CLI API and implementation doesn't change that much).
However, I noticed that address show test is missing, so I added at least that. I tried to add a test for both payment path and script hash present (which violates the mutual exclusiveness), but it turns out that when argparse founds a problem it terminates the process instead of throwing an error. There is an option exit_on_error which can be set to false, but I couldn't get it working with mutually exclusive groups correctly. Again, I'm not sure it's worth the effort and added complexity in code.
There was a problem hiding this comment.
I don't think we need to test individual parsers.
At this point, I would leave this as an open github issue. We can return to it someday, perhaps after argparse will start working as expected :) Just write down a concise summary of why you couldn't make it work in the issue.
There was a problem hiding this comment.
I agree, issue added #111
davidmisiak
force-pushed
the
dm/106-mutually-exclusive-arguments
branch
from
955aadb to
bb13a5b
Compare
davidmisiak
force-pushed
the
dm/102-refactor-signing-validation
branch
from
aad00db to
81ea3f7
Compare
davidmisiak
force-pushed
the
dm/106-mutually-exclusive-arguments
branch
from
bb13a5b to
a5d6a30
Compare
davidmisiak
changed the base branch from
dm/102-refactor-signing-validation
to
ja/multisig
Fixes #106.
The
_mutually-exclusive-groupkey feels somewhat hacky, but I think it could be acceptable, because it keeps theparseConfig.tsreadability and doesn't require something needlessly complicated incommandParser.ts.