util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet #20715

maflcko · 2020-12-18T14:07:13Z

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

DrahtBot · 2020-12-18T16:57:11Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: Replace fs::unique_path with GetUniquePath(path) calls #21052 (refactor: Replace fs::unique_path with GetUniquePath(path) calls by kiminuo)
multiprocess: Add bitcoin-wallet -ipcconnect option #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)

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.

practicalswift · 2020-12-19T09:30:27Z

Concept ACK

ajtowns

Concept ACK

src/util/system.cpp

src/bitcoin-wallet.cpp

DrahtBot · 2021-01-13T11:46:31Z

🕵️ @jonatack has been requested to review this pull request as specified in the REVIEWERS file.

ajtowns

ACK fad369f

Looks good; comments are just ideas for consideration. Patch for bitcoin-util at https://github.com/ajtowns/bitcoin/commits/202101-util-addcmd

src/util/system.cpp

src/bitcoin-wallet.cpp

fjahr

Code review ACK fad369f

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?

src/bitcoin-wallet.cpp

laanwj · 2021-01-20T12:53:31Z

Concept ACK

Co-Authored-by: Anthony Towns <aj@erisian.com.au>

fjahr

Code review ACK fa61b9d

Feel free to ignore my nits.

fjahr · 2021-01-23T23:29:49Z

test/functional/tool_wallet.py

@@ -188,6 +188,8 @@ def test_invalid_tool_commands_and_args(self):
        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.')


in fa06bce:

nit: Commit message could have been a bit more descriptive ;)

Thanks, will change on the next push

fjahr · 2021-01-23T23:38:52Z

src/util/system.h

@@ -248,6 +251,20 @@ class ArgsManager
     */
    const std::list<SectionInfo> GetUnrecognizedSections() const;

+    struct Command {


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.

#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.

ajtowns · 2021-02-04T05:27:04Z

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

maflcko · 2021-02-04T08:13:56Z

third commit has a typo in the description: "dependend".

Good catch. Though, I'll leave this as-is to minimize the re-review backlog.

maflcko force-pushed the 2012-argsCmd branch 2 times, most recently from faeba84 to fa6cd35 Compare December 18, 2020 14:14

DrahtBot added the Utils/log/libs label Dec 18, 2020

maflcko force-pushed the 2012-argsCmd branch from fa6cd35 to faaf1b2 Compare December 18, 2020 17:40

This was referenced Dec 18, 2020

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

refactor: Implement missing error checking for ArgsManager flags #16545

Draft

DrahtBot mentioned this pull request Jan 2, 2021

fuzz: Introduce CallOneOf helper to replace switch-case #20828

Merged

maflcko mentioned this pull request Jan 11, 2021

wallet tool silently ignores trailing options #20902

Closed

ajtowns reviewed Jan 12, 2021

View reviewed changes

src/util/system.cpp Outdated Show resolved Hide resolved

src/util/system.cpp Outdated Show resolved Hide resolved

src/bitcoin-wallet.cpp Outdated Show resolved Hide resolved

ajtowns mentioned this pull request Jan 12, 2021

signet mining utility #19937

Merged

maflcko force-pushed the 2012-argsCmd branch from faaf1b2 to fab1df0 Compare January 13, 2021 09:13

maflcko changed the title ~~util: Add ArgsManager::GetCommands() and use it in bitcoin-wallet~~ util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet Jan 13, 2021

maflcko force-pushed the 2012-argsCmd branch from fab1df0 to fafe392 Compare January 13, 2021 12:17

DrahtBot added the Needs rebase label Jan 14, 2021

maflcko force-pushed the 2012-argsCmd branch from fafe392 to fad369f Compare January 14, 2021 10:31

DrahtBot removed the Needs rebase label Jan 14, 2021

ajtowns reviewed Jan 15, 2021

View reviewed changes

src/util/system.cpp Outdated Show resolved Hide resolved

src/util/system.cpp Outdated Show resolved Hide resolved

src/bitcoin-wallet.cpp Outdated Show resolved Hide resolved

fjahr reviewed Jan 17, 2021

View reviewed changes

src/bitcoin-wallet.cpp Outdated Show resolved Hide resolved

src/bitcoin-wallet.cpp Outdated Show resolved Hide resolved

src/bitcoin-wallet.cpp Show resolved Hide resolved

maflcko force-pushed the 2012-argsCmd branch from fad369f to fa76c78 Compare January 21, 2021 12:54

DrahtBot mentioned this pull request Jan 21, 2021

refactor: Replace fs::absolute calls with AbsPathJoin calls #20932

Merged

DrahtBot added the Needs rebase label Jan 21, 2021

MarcoFalke and others added 4 commits January 21, 2021 19:29

wallet: [refactor] Pass ArgsManager to WalletAppInit

fac05cc

test: Add tests

fa06bce

refactor: Move all command dependend checks to ExecuteWalletToolFunc

7777105

util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet

fa61b9d

Co-Authored-by: Anthony Towns <aj@erisian.com.au>

maflcko force-pushed the 2012-argsCmd branch from fa76c78 to fa61b9d Compare January 21, 2021 18:31

DrahtBot removed the Needs rebase label Jan 21, 2021

fjahr approved these changes Jan 23, 2021

View reviewed changes

DrahtBot mentioned this pull request Feb 1, 2021

refactor: Replace fs::unique_path with GetUniquePath(path) calls #21052

Merged

maflcko merged commit 4e946eb into bitcoin:master Feb 4, 2021

maflcko deleted the 2012-argsCmd branch February 4, 2021 08:13

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 4, 2021

Merge bitcoin#20715: util: Add ArgsManager::GetCommand() and use it

921561a

ajtowns mentioned this pull request Feb 6, 2021

signet miner followups #20923

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet #20715

util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet #20715

Uh oh!

Conversation

maflcko commented Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift commented Dec 19, 2020

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Jan 13, 2021

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laanwj commented Jan 20, 2021

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr Jan 23, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 24, 2021

Choose a reason for hiding this comment

Uh oh!

fjahr Jan 23, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 24, 2021

Choose a reason for hiding this comment

Uh oh!

ajtowns commented Feb 4, 2021

Uh oh!

maflcko commented Feb 4, 2021

Uh oh!

Uh oh!

maflcko commented Dec 18, 2020 •

edited

Loading

DrahtBot commented Dec 18, 2020 •

edited

Loading