Skip to content
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.

Sign up for GitHub

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

Jump to bottom

doc: Clarify rpcwallet flag url change #14448

Merged
merged 1 commit into from Nov 23, 2018
Merged

doc: Clarify rpcwallet flag url change #14448

merged 1 commit into from Nov 23, 2018

Conversation

JBaczuk
Copy link
Contributor

@JBaczuk JBaczuk commented Oct 9, 2018

This adds clarification to the bitcoin-cli -rpcwallet flag in the help command. This will benefit users who want to utilize this feature without the cli, for example curl. It isn't readily apparent that this changes the url used in the RPC call.

promag
promag reviewed Oct 9, 2018
View reviewed changes
Copy link
Member

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe improve documentation like create doc/RPC-interface.md.

src/bitcoin-cli.cpp
@@ -51,7 +51,7 @@ static void SetupCliArgs()
gArgs.AddArg("-rpcport=<port>", strprintf("Connect to JSON-RPC on <port> (default: %u, testnet: %u, regtest: %u)", defaultBaseParams->RPCPort(), testnetBaseParams->RPCPort(), regtestBaseParams->RPCPort()), false, OptionsCategory::OPTIONS);
gArgs.AddArg("-rpcuser=<user>", "Username for JSON-RPC connections", false, OptionsCategory::OPTIONS);
gArgs.AddArg("-rpcwait", "Wait for RPC server to start", false, OptionsCategory::OPTIONS);
gArgs.AddArg("-rpcwallet=<walletname>", "Send RPC for non-default wallet on RPC server (needs to exactly match corresponding -wallet option passed to bitcoind)", false, OptionsCategory::OPTIONS);
gArgs.AddArg("-rpcwallet=<walletname>", "Send RPC for non-default wallet on RPC server (needs to exactly match corresponding -wallet option passed to bitcoind). This changes the RPC endpoint used, e.g. http://127.0.0.1:8332/wallet/<walletname>", false, OptionsCategory::OPTIONS);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this relevant from the bitcoin-cli user's point of view? At least I'd drop the e.g. ... part here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the best place for this, but I think an example url would be helpful somewhere. There are curl examples for each command help, but not for any of the flags.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with meshcollider and promag, and think this information is useful. Or at least I don't understand how it harms anyone. I hate documentation that just gives me a handwavy gloss about how an option is intended to be used without saying what it actually does.

Of course it would also be good to document wallet endpoints in markdown. There is some text in the 0.15.0 release notes ("HTTP RPC requests should be send to the...") that could be moved to JSON-RPC-interface.md. This could be done here or in a separate PR. I actually remember complaining about lack of API documentation when this feature was first added #10849 (comment), and I guess it still doesn't exist.

Copy link
Member

@laanwj laanwj Nov 23, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally don't think the option help is the place to put very detailed specifications (just like the release notes are not !), but in this case I kind of like it because it's short and directly illustrates what it does.

@fanquake fanquake added the Docs label Oct 9, 2018
@promag promag mentioned this pull request Oct 10, 2018
Closed
@meshcollider
Copy link
Contributor

Agree with @promag, IMO this isn't the right place to put such an example, it would be better in some other documentation not in bitcoin-cli help text

@JBaczuk
Copy link
Contributor Author

JBaczuk commented Nov 9, 2018 •
edited

I agree, maybe doc/RPC-interface.md is a better solution, the only problem is that many users will not know about the new doc. Perhaps the incentive to dig a little is great enough. I wonder if the doc/RPC-interface.md would be too difficult to maintain, e.g. the manpages are is generated deterministically.

ryanofsky
ryanofsky approved these changes Nov 12, 2018
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 0c69ff6

src/bitcoin-cli.cpp
@@ -51,7 +51,7 @@ static void SetupCliArgs()
gArgs.AddArg("-rpcport=<port>", strprintf("Connect to JSON-RPC on <port> (default: %u, testnet: %u, regtest: %u)", defaultBaseParams->RPCPort(), testnetBaseParams->RPCPort(), regtestBaseParams->RPCPort()), false, OptionsCategory::OPTIONS);
gArgs.AddArg("-rpcuser=<user>", "Username for JSON-RPC connections", false, OptionsCategory::OPTIONS);
gArgs.AddArg("-rpcwait", "Wait for RPC server to start", false, OptionsCategory::OPTIONS);
gArgs.AddArg("-rpcwallet=<walletname>", "Send RPC for non-default wallet on RPC server (needs to exactly match corresponding -wallet option passed to bitcoind)", false, OptionsCategory::OPTIONS);
gArgs.AddArg("-rpcwallet=<walletname>", "Send RPC for non-default wallet on RPC server (needs to exactly match corresponding -wallet option passed to bitcoind). This changes the RPC endpoint used, e.g. http://127.0.0.1:8332/wallet/<walletname>", false, OptionsCategory::OPTIONS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with meshcollider and promag, and think this information is useful. Or at least I don't understand how it harms anyone. I hate documentation that just gives me a handwavy gloss about how an option is intended to be used without saying what it actually does.

Of course it would also be good to document wallet endpoints in markdown. There is some text in the 0.15.0 release notes ("HTTP RPC requests should be send to the...") that could be moved to JSON-RPC-interface.md. This could be done here or in a separate PR. I actually remember complaining about lack of API documentation when this feature was first added #10849 (comment), and I guess it still doesn't exist.

@laanwj
Copy link
Member

laanwj commented Nov 23, 2018

utACK 0c69ff6

@laanwj laanwj changed the title clarify rpcwallet flag url change doc: Clarify rpcwallet flag url change Nov 23, 2018
@laanwj laanwj merged commit 0c69ff6 into bitcoin:master Nov 23, 2018
laanwj added a commit that referenced this pull request Nov 23, 2018
@laanwj
Merge #14448: doc: Clarify rpcwallet flag url change
e3a1b74 
0c69ff6 clarify rpcwallet flag url change (Jordan Baczuk)

Pull request description:

  This adds clarification to the bitcoin-cli -rpcwallet flag in the help command. This will benefit users who want to utilize this feature without the cli, for example curl. It isn't readily apparent that this changes the url used in the RPC call.

Tree-SHA512: 6fc759f193f0a918884aab8ba4dc77ed9e89ee3840feeff737a754be758750590f5bd44b40f4810c3b82601e125e62e10360af45cb8e9d95be206ebeb9120ebf
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 10, 2020
@JBaczuk @PiRK
clarify rpcwallet flag url change
7b2fda0 
Summary: This is a backport of Core [[bitcoin/bitcoin#14448 | PR14448]]

Test Plan: `src/bitcoin-cli -help`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D8351
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 27, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14448: doc: Clarify rpcwallet flag url change
94cc076 
0c69ff6 clarify rpcwallet flag url change (Jordan Baczuk)

Pull request description:

  This adds clarification to the bitcoin-cli -rpcwallet flag in the help command. This will benefit users who want to utilize this feature without the cli, for example curl. It isn't readily apparent that this changes the url used in the RPC call.

Tree-SHA512: 6fc759f193f0a918884aab8ba4dc77ed9e89ee3840feeff737a754be758750590f5bd44b40f4810c3b82601e125e62e10360af45cb8e9d95be206ebeb9120ebf
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 28, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14448: doc: Clarify rpcwallet flag url change
be0dd35 
0c69ff6 clarify rpcwallet flag url change (Jordan Baczuk)

Pull request description:

  This adds clarification to the bitcoin-cli -rpcwallet flag in the help command. This will benefit users who want to utilize this feature without the cli, for example curl. It isn't readily apparent that this changes the url used in the RPC call.

Tree-SHA512: 6fc759f193f0a918884aab8ba4dc77ed9e89ee3840feeff737a754be758750590f5bd44b40f4810c3b82601e125e62e10360af45cb8e9d95be206ebeb9120ebf
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14448: doc: Clarify rpcwallet flag url change
b728b7f 
0c69ff6 clarify rpcwallet flag url change (Jordan Baczuk)

Pull request description:

  This adds clarification to the bitcoin-cli -rpcwallet flag in the help command. This will benefit users who want to utilize this feature without the cli, for example curl. It isn't readily apparent that this changes the url used in the RPC call.

Tree-SHA512: 6fc759f193f0a918884aab8ba4dc77ed9e89ee3840feeff737a754be758750590f5bd44b40f4810c3b82601e125e62e10360af45cb8e9d95be206ebeb9120ebf
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14448: doc: Clarify rpcwallet flag url change
097f696 
0c69ff6 clarify rpcwallet flag url change (Jordan Baczuk)

Pull request description:

  This adds clarification to the bitcoin-cli -rpcwallet flag in the help command. This will benefit users who want to utilize this feature without the cli, for example curl. It isn't readily apparent that this changes the url used in the RPC call.

Tree-SHA512: 6fc759f193f0a918884aab8ba4dc77ed9e89ee3840feeff737a754be758750590f5bd44b40f4810c3b82601e125e62e10360af45cb8e9d95be206ebeb9120ebf
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14448: doc: Clarify rpcwallet flag url change
71834bb 
0c69ff6 clarify rpcwallet flag url change (Jordan Baczuk)

Pull request description:

  This adds clarification to the bitcoin-cli -rpcwallet flag in the help command. This will benefit users who want to utilize this feature without the cli, for example curl. It isn't readily apparent that this changes the url used in the RPC call.

Tree-SHA512: 6fc759f193f0a918884aab8ba4dc77ed9e89ee3840feeff737a754be758750590f5bd44b40f4810c3b82601e125e62e10360af45cb8e9d95be206ebeb9120ebf
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14448: doc: Clarify rpcwallet flag url change
fb3dd1c 
0c69ff6 clarify rpcwallet flag url change (Jordan Baczuk)

Pull request description:

  This adds clarification to the bitcoin-cli -rpcwallet flag in the help command. This will benefit users who want to utilize this feature without the cli, for example curl. It isn't readily apparent that this changes the url used in the RPC call.

Tree-SHA512: 6fc759f193f0a918884aab8ba4dc77ed9e89ee3840feeff737a754be758750590f5bd44b40f4810c3b82601e125e62e10360af45cb8e9d95be206ebeb9120ebf
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Jul 5, 2021
@laanwj @UdjinM6
Merge bitcoin#14448: doc: Clarify rpcwallet flag url change
506ef80 
0c69ff6 clarify rpcwallet flag url change (Jordan Baczuk)

Pull request description:

  This adds clarification to the bitcoin-cli -rpcwallet flag in the help command. This will benefit users who want to utilize this feature without the cli, for example curl. It isn't readily apparent that this changes the url used in the RPC call.

Tree-SHA512: 6fc759f193f0a918884aab8ba4dc77ed9e89ee3840feeff737a754be758750590f5bd44b40f4810c3b82601e125e62e10360af45cb8e9d95be206ebeb9120ebf
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

@promag promag promag left review comments

@ryanofsky ryanofsky ryanofsky approved these changes

Assignees
No one assigned
Labels
Docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@JBaczuk @meshcollider @laanwj @promag @ryanofsky @fanquake