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.

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

rpc: Document default values for optional arguments #14877

Merged
merged 1 commit into from Dec 10, 2018

Conversation

Projects
None yet
6 participants
@MarcoFalke
Copy link
Member

commented Dec 5, 2018

No description provided.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)
  • #14641 (rpc: Add min_conf option to fund transaction calls by promag)
  • #11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)

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.

Show resolved Hide resolved src/rpc/rawtransaction.cpp Outdated
Show resolved Hide resolved src/rpc/rawtransaction.cpp Outdated
Show resolved Hide resolved src/rpc/rawtransaction.cpp Outdated

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-rpcDefault branch Dec 6, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

Thanks @practicalswift. Fixed my language

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-rpcDefault branch to fa0c24c Dec 7, 2018

@DrahtBot DrahtBot removed the Needs rebase label Dec 7, 2018

@@ -888,9 +888,9 @@ static UniValue estimaterawfee(const JSONRPCRequest& request)
"defined in BIP 141 (witness data is discounted).\n",
{
{"conf_target", RPCArg::Type::NUM, /* opt */ false, /* default_val */ "", "Confirmation target in blocks (1 - 1008)"},
{"threshold", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "", "The proportion of transactions in a given feerate range that must have been\n"
{"threshold", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "0.95", "The proportion of transactions in a given feerate range that must have been\n"

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Dec 9, 2018

Member

what about using a stringprint of the threshold variable?

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Dec 9, 2018

utACK fa0c24c
Somehow I would be in favour of putting brackets around descriptive values ((null) instead of null and (fallback to nodeid) instead of fallback to nodeid) to avoid confusion between real default values and behavioural description.

@kristapsk

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2018

ACK fa0c24c

@ryanofsky
Copy link
Contributor

left a comment

ACK fa0c24c. I checked help output using the hack from #14726 (review)

This is a nice documentation change. If you wanted to follow up with more improvements I have some suggestions below:

  • The fallback descriptions like "fallback to wallet's default" are vague and could be described in more detail.
  • I don't think saying (default=null) or (default=omitted) is ever helpful. Just because of the way we check arguments, omitting an argument is always equivalent to passing null. And (default=omitted) is just a tautology. The point of the default documentation should be to say what an RPC call will do when an argument is null or omitted. I actually think it would be better to drop the all the (default=null) and (default=omitted) strings if they can't be replaced with something more constructive, to avoid creating a bad precedent for future changes.
{"bantime", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "", "time in seconds how long (or until when if [absolute] is set) the IP is banned (0 or empty means using the default time of 24h which can also be overwritten by the -bantime startup argument)"},
{"absolute", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "", "If set, the bantime must be an absolute timestamp in seconds since epoch (Jan 1 1970 GMT)"},
{"bantime", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "0", "time in seconds how long (or until when if [absolute] is set) the IP is banned (0 or empty means using the default time of 24h which can also be overwritten by the -bantime startup argument)"},
{"absolute", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "false", "If set, the bantime must be an absolute timestamp in seconds since epoch (Jan 1 1970 GMT)"},

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 10, 2018

Contributor

/If set/If true/ might be more accurate

@kristapsk

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2018

I don't think saying (default=null) or (default=omitted) is ever helpful. (...) I actually think it would be better to drop the all the (default=null) and (default=omitted) strings if they can't be replaced with something more constructive, to avoid creating a bad precedent for future changes.

If there is "default=" string outputted for each optional argument, functional test could be made that checks that people don't forget to document optional argument defaults for a new RPCs.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2018

If there is "default=" string outputted for each optional argument, functional test could be made that checks that people don't forget to document optional argument defaults for a new RPCs.

IMO, better to write that test with a list of exceptions for grandfathered, undocumented default arguments, than to leave bad documentation scattered over the codebase, especially since existing documentation is often used as a template for new documentation

@kristapsk

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2018

Oh, yes, short list of a few exceptions probably is better idea.

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Dec 10, 2018

I have changes piled up locally to enforce that it is either a required arg or a default value is provided at compile time.

@MarcoFalke MarcoFalke merged commit fa0c24c into bitcoin:master Dec 10, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Dec 10, 2018

Merge #14877: rpc: Document default values for optional arguments
fa0c24c rpc: Document default values for optional arguments (MarcoFalke)

Pull request description:

Tree-SHA512: e1f5ea67d7ac67526ae87bffaeb308a9ad68632e161fe0148cd431a340bb7a30def18f1dbc7e98c6c1c269ac8942fd5d5334c85c48e4fb1cead70a42536b6eef

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1812-rpcDefault branch Dec 10, 2018

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2018

@MarcoFalke what do you want to do with all the (default=null) / (default=omitted) cruft? Do you want to address it in a future PR or maybe mark this one up for grabs so someone else can fill in useful default values?

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Dec 10, 2018

@ryanofsky Your suggestion is a one-line change after #14918, so either you leave feedback there or create a pull request after #14918 is merged.

MarcoFalke added a commit that referenced this pull request Jan 30, 2019

Merge #15279: wallet: Clarify rescanblockchain doc
fa5e6ef wallet: Fixup rescanblockchain result doc (MarcoFalke)

Pull request description:

  This was probably accidentally added to the wrong line when addressing the feedback here: #7061 (comment)

  I already added the default values in #14877, but it could be clarified more that this really has no specific block height as default value, since the tip can change during a rescan.

Tree-SHA512: 48a3c5143e2b7129ee8f396d2e77550cb393fbe45f5936aeebeb7a201d61560336a3ae47b26bb757a4dbbe217e06abfd67a5a673aef266b6c4d7a80d049a2b49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.