util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet #20715
Conversation
maflcko
force-pushed
the
2012-argsCmd
branch
2 times, most recently
from
faeba84 to
fa6cd35
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
maflcko
force-pushed
the
2012-argsCmd
branch
from
fa6cd35 to
faaf1b2
Compare
|
Concept ACK |
maflcko
changed the title
|
🕵️ @jonatack has been requested to review this pull request as specified in the REVIEWERS file. |
ajtowns
left a comment
ajtowns
left a comment
There was a problem hiding this comment.
ACK fad369f04361e2e3a834589b63fab160b9b4e1b2
Looks good; comments are just ideas for consideration. Patch for bitcoin-util at https://github.com/ajtowns/bitcoin/commits/202101-util-addcmd
fjahr
left a comment
fjahr
left a comment
There was a problem hiding this comment.
Code review ACK fad369f04361e2e3a834589b63fab160b9b4e1b2
This works, however I liked @ajtowns comments and I think consistency whether method or command is used would be good. It seems method is getting replaced by command, is that a general naming scheme change?
|
Concept ACK |
| self.assert_raises_tool_error('Invalid command: help', 'help') | ||
| self.assert_raises_tool_error('Error: two methods provided (info and create). Only one method should be provided.', 'info', 'create') | ||
| self.assert_raises_tool_error('Error parsing command line arguments: Invalid parameter -foo', '-foo') | ||
| self.assert_raises_tool_error('No method provided. Run `bitcoin-wallet -help` for valid methods.') |
There was a problem hiding this comment.
in fa06bce:
nit: Commit message could have been a bit more descriptive ;)
There was a problem hiding this comment.
Thanks, will change on the next push
| */ | ||
| const std::list<SectionInfo> GetUnrecognizedSections() const; | ||
|
|
||
| struct Command { |
There was a problem hiding this comment.
in fa61b9d:
nit: seeing this now I think I would have named it CommandCall or something similar to express that it's command + args. But feel free to ignore my name bikeshedding.
There was a problem hiding this comment.
#20715 (comment) names it CmdArg. So I could call it CommandArg or CommandArgs (because there might be multiple args to the command). Not sure what to do here, but I think I'll leave this as is for now and let the naming committee determine a suitable name and let them change it after merge.
|
ACK fa61b9d Looks good to me, though the third commit has a typo in the description: "dependend". Updated https://github.com/ajtowns/bitcoin/commits/202101-util-addcmd |
Good catch. Though, I'll leave this as-is to minimize the re-review backlog. |
6 participants
This not only moves the parsing responsibility out from the wallet tool, but it also makes it easier to implement bitcoin-util #19937
Fixes: #20902