docs: Consistent type names in RPC help descriptions #14459

ch4ot1c commented at 5:51 PM on October 10, 2018: contributor

Followup to #14373.

Now, only these appear in description text:

boolean
numeric
string

json array
json array of xs
json object
json object of xs

ch4ot1c force-pushed on Oct 10, 2018

fanquake added the label Docs on Oct 10, 2018

fanquake added the label RPC/REST/ZMQ on Oct 10, 2018

ch4ot1c force-pushed on Oct 10, 2018

in src/wallet/rpcwallet.cpp:687 in 023586cb48 outdated

 683 | @@ -684,12 +684,12 @@ static UniValue getbalance(const JSONRPCRequest& request)
 684 |  
 685 |      if (request.fHelp || (request.params.size() > 3 ))
 686 |          throw std::runtime_error(
 687 | -            "getbalance ( \"(dummy)\" minconf include_watchonly )\n"
 688 | +            "getbalance ( dummy minconf include_watchonly )\n"

promag commented at 12:07 AM on October 11, 2018:

dummy is a string.

ch4ot1c commented at 12:28 AM on October 11, 2018: contributor

See above comment.

DrahtBot commented at 10:03 AM on October 20, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14987 (RPCHelpMan: Pass through Result and Examples by MarcoFalke)
#14918 (RPCHelpMan: Check default values are given at compile-time by MarcoFalke)
#14912 ([WIP] External signer support (e.g. hardware wallet) by Sjors)
#14481 (Add P2SH-P2WSH support to listunspent RPC by MeshCollider)
#14021 (Import key origin data through descriptors in importmulti by achow101)
#12911 (wallet: Show fee in results for signrawtransaction* for segwit inputs 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.

MarcoFalke commented at 2:32 AM on October 21, 2018: member

importprunedfunds has arguments, but they are not listed in the help string. Mind adding them?
uptime takes (and ignores) a dummy argument, whereas it shouldn't. Mind fixing the code to throw when a dummy is provided?

in src/rpc/server.cpp:252 in 91ed9518ea outdated

 244 | @@ -245,6 +245,11 @@ static UniValue uptime(const JSONRPCRequest& jsonRequest)
 245 |                  + HelpExampleRpc("uptime", "")
 246 |          );
 247 |  
 248 | +    const UniValue& dummy_value = jsonRequest.params[0];
 249 | +    if (!dummy_value.isNull()) {
 250 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, "No dummy first argument may be included.");
 251 | +    }
 252 | +

MarcoFalke commented at 4:51 AM on October 21, 2018:

Can be fixed instead by replacing > 1 with > 0 a few lines up?

ch4ot1c commented at 4:59 AM on October 21, 2018:

Ah, that was what I thought at first.

MarcoFalke commented at 4:52 AM on October 21, 2018: member

prioritisetransaction has an argument with a space. Mind to replace the space with underscore? getbalance has an argument name with a opening bracket. Mind to remove those brackets from the name?

in src/wallet/rpcdump.cpp:1113 in 91ed9518ea outdated

1112 | @@ -1108,8 +1113,8 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
1113 |              "importmulti \"requests\" ( \"options\" )\n\n"

MarcoFalke commented at 4:55 AM on October 21, 2018:

             "importmulti \"requests\" ( \"options\" )\n"

ch4ot1c force-pushed on Oct 21, 2018

ch4ot1c commented at 5:08 AM on October 21, 2018: contributor

~prioritisetransaction and getbalance changes are already in the PR.~

Edit: All above issues by MarcoFalke have been resolved by the latest code having transitioned to RPCHelpMan.

in src/zmq/zmqrpc.cpp:25 in bd88f6b253 outdated

  19 | @@ -20,7 +20,7 @@ UniValue getzmqnotifications(const JSONRPCRequest& request)
  20 |              "\nReturns information about the active ZeroMQ notifications.\n"
  21 |              "\nResult:\n"
  22 |              "[\n"
  23 | -            "  {                        (json object)\n"
  24 | +            "  {                        (object)\n"

MarcoFalke commented at 5:35 AM on October 21, 2018:

Half of the changes seem to be of this fashion. Mind to create a primitive scripted-diff commit in a separate pull request to get the easy to review things out of the way?

ch4ot1c commented at 8:42 AM on October 21, 2018:

Damn, they need to make a sed-builder plugin alongside find-and-replace in vscode, that supports negation. Way easier workflow for newer contributors than all the nuanced escape characters that are necessary in *nix tools.

Is it preferred to use git grep (-l) (-L) or find . x in scripted diffs?

~Also, does this really make it easier to review for correctness? I'm not so sure.~ Verification is always good

Next commit will be a scripted diff for:

Replace (boolean)W with (boolean) W in .

git grep -l "(boolean)W" | xargs sed -i "s/(boolean)W/(boolean) W/g" is about right I think?

What if I'm stuck in PowerShell! No xargs available.

ch4ot1c commented at 8:51 AM on October 21, 2018:

And on Mac, gsed is needed to replicate sed behavior performed in scripted-diffs

MarcoFalke commented at 1:30 PM on October 21, 2018:

I'd do it like this: sed -i --regexp-extended -e 's/\(json (array|object)/(\1/g' $(git grep -l '(json '). There might be a hack to make it work on macOS, but we mostly care that it runs on Ubuntu et al.

ch4ot1c force-pushed on Oct 22, 2018

DrahtBot added the label Needs rebase on Nov 13, 2018

MarcoFalke referenced this in commit 8c59bb85f9 on Nov 13, 2018

MarcoFalke commented at 9:53 PM on November 13, 2018: member

Could rebase and squash with something like:

git fetch bitcoin
git checkout fix/rpc-descriptions
git merge bitcoin/master
git reset --soft bitcoin/master
git commit -m "More RPC help description fixes"
git push origin fix/rpc-descriptions --force

DrahtBot removed the label Needs rebase on Nov 14, 2018

DrahtBot added the label Needs rebase on Nov 23, 2018

ch4ot1c closed this on Nov 25, 2018

ch4ot1c force-pushed on Nov 25, 2018

ch4ot1c reopened this on Nov 25, 2018

ch4ot1c force-pushed on Nov 25, 2018

DrahtBot removed the label Needs rebase on Nov 25, 2018

DrahtBot added the label Needs rebase on Nov 26, 2018

ch4ot1c closed this on Dec 2, 2018

ch4ot1c force-pushed on Dec 2, 2018

ch4ot1c reopened this on Dec 7, 2018

ch4ot1c force-pushed on Dec 7, 2018

DrahtBot removed the label Needs rebase on Dec 7, 2018

practicalswift commented at 12:28 PM on December 8, 2018: contributor

This PR doesn't compile when rebased on master

ch4ot1c force-pushed on Dec 8, 2018

laanwj commented at 12:11 PM on December 13, 2018: member

utACK 2f6fff743a2f1e7c51bee41c18498b4043ba9e48

MarcoFalke commented at 5:34 PM on December 13, 2018: member

src/rpc/util.cpp uses "json object". Imo everything should use the same

ch4ot1c commented at 10:01 PM on December 13, 2018: contributor

@MarcoFalke ~I see. To confirm, json array and json object everywhere applicable, preferred over array and object, aka util.cpp is set in stone? Will redo as a scripted-diff once more.~

Edit: Done. Note: an array and an object is still used in bitcoin-core/univalue, and corresponding BTC tests. Additionally, doc/REST-interface.md still uses (object) and (array), since it mentions JSON beforehand.

ch4ot1c force-pushed on Dec 13, 2018

ch4ot1c force-pushed on Dec 14, 2018

in src/rpc/mining.cpp:317 in 8f3ed05b1f outdated

 313 | @@ -314,12 +314,12 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
 314 |                      {"template_request", RPCArg::Type::OBJ, /* opt */ true, /* default_val */ "", "A json object in the following spec",
 315 |                          {
 316 |                              {"mode", RPCArg::Type::STR, /* opt */ true, /* default_val */ "", "This must be set to \"template\", \"proposal\" (see BIP 23), or omitted"},
 317 | -                            {"capabilities", RPCArg::Type::ARR, /* opt */ true, /* default_val */ "", "A list of strings",
 318 | +                            {"capabilities", RPCArg::Type::ARR, /* opt */ true, /* default_val */ "", "An array of strings",

ch4ot1c commented at 5:20 PM on December 14, 2018:

A json array here instead?

DrahtBot added the label Needs rebase on Dec 21, 2018

ch4ot1c force-pushed on Dec 21, 2018

ch4ot1c force-pushed on Dec 28, 2018

DrahtBot removed the label Needs rebase on Dec 28, 2018

ch4ot1c force-pushed on Dec 28, 2018

scripted-diff: [rpc] Descriptions: 'array of' -> 'json array of'

-BEGIN VERIFY SCRIPT-

git grep -l "(array of" src | xargs sed -i "s/(array of/(json array of/g"

-END VERIFY SCRIPT-

4fd7c6ae25

scripted-diff: [rpc] Descriptions: 'of Objects' -> 'of json objects'

-BEGIN VERIFY SCRIPT-

git grep -l "of Objects" src | xargs sed -i "s/of Objects/of json objects/g"

-END VERIFY SCRIPT-

10e5b2d698

scripted-diff: [rpc] Descriptions: Fix usage of 'list' instead of 'array'

-BEGIN VERIFY SCRIPT-

git grep -l "(list of objects)" src | xargs sed -i "s/(list of objects)/(json array of json objects)/g"

-END VERIFY SCRIPT-

6741bcaaa3

scripted-diff: [rpc] Descriptions: '(bool) ' -> '(boolean) '

-BEGIN VERIFY SCRIPT-

git grep -l "(bool) " src | xargs sed -i "s/(bool) /(boolean) /g"

-END VERIFY SCRIPT-

1e758b75b4

scripted-diff: [rpc] Descriptions: 'of objects' -> 'of json objects'

-BEGIN VERIFY SCRIPT-

git grep -l "of objects[)\.]" src/rpc src/wallet | xargs sed -i "s/of objects/of json objects/g"

-END VERIFY SCRIPT-

8bf04532b4

scripted-diff: [rpc] Descriptions: ' an object' -> ' a json object'

-BEGIN VERIFY SCRIPT-

git grep -l " an object" src/rpc src/wallet | xargs sed -i "s/ an object/ a json object/g"

-END VERIFY SCRIPT-

ee8f93a93d

scripted-diff: [rpc] Descriptions: 'of string)' -> 'of strings)'

-BEGIN VERIFY SCRIPT-

git grep -l "of string)" src/rpc src/wallet | xargs sed -i "s/of string)/of strings)/g"

-END VERIFY SCRIPT-

ff7aefe4c0

scripted-diff: [rpc] Descriptions: 'A list of ' -> 'An array of ' in mining.cpp

-BEGIN VERIFY SCRIPT-

git grep -l "A list of " src/rpc/mining.cpp | xargs sed -i "s/A list of /An array of /g"

-END VERIFY SCRIPT-

cc64db060d

[rpc] Descriptions: (int) -> (numeric) in blockchain.cpp 8ab41f7cff

[rpc] Descriptions: 'integer(s)' -> 'numeric(s)' d4a0b342cd

[rpc] Descriptions: Use 'json object' instead of 'dict' in util.h f6e1b48908

scripted-diff: [rpc] Descriptions: 'Must be one of' -> 'Must be one of:'

-BEGIN VERIFY SCRIPT-

git grep -l "Must be one of\\\n" src | xargs sed -i "s/Must be one of\\\n/Must be one of:\\\n/g"

-END VERIFY SCRIPT-

4b6928003e

scripted-diff: [rpc] Descriptions: 'an array' -> 'a json array'

-BEGIN VERIFY SCRIPT-

git grep -l "[aA]n array" src/rpc src/wallet | xargs sed -i "s/an array/a json array/g"

git grep -l "[aA]n array" src/rpc src/wallet | xargs sed -i "s/An array/A json array/g"

-END VERIFY SCRIPT-

db51182525

scripted-diff: [rpc] Tests: 'pair not an object' -> 'pair not a json object'

-BEGIN VERIFY SCRIPT-

git grep -l "pair not an object" test/functional/rpc_rawtransaction.py | xargs sed -i "s/pair not an object/pair not a json object/g"

-END VERIFY SCRIPT-

ff7329fe34

ch4ot1c force-pushed on Dec 28, 2018

ch4ot1c commented at 9:28 PM on December 28, 2018: contributor

Rebased and ready for review @MarcoFalke. Let me know how you'd like this squashed.

scripted-diff: [rpc] Descriptions: 'of numeric' -> 'of numerics' in blockchain.cpp

-BEGIN VERIFY SCRIPT-

git grep -l "of numeric" src/rpc/blockchain.cpp | xargs sed -i "s/of numeric/of numerics/g"

-END VERIFY SCRIPT-

99ab26bb99

scripted-diff: [rpc] Descriptions: '(object) ' -> '(json object) ' in src/rpc, src/wallet

-BEGIN VERIFY SCRIPT-

git grep -l "(object) " src/wallet src/rpc | xargs sed -i "s/(object) /(json object) /g"

-END VERIFY SCRIPT-

1e96af507f

fanquake renamed this:
~~More RPC help description fixes~~
docs: More RPC help description fixes
on Dec 29, 2018

ch4ot1c renamed this:
~~docs: More RPC help description fixes~~
docs: Consistent type names in RPC help descriptions
on Dec 29, 2018

laanwj commented at 4:43 PM on January 14, 2019: member

Concept ACK.

Though I think the jargon switch to numeric loses information.

The code, in practice, makes a difference between numeric (in general) and integer. For example where get_int is used, only an integer will be accepted. Not a decimal value with a point.

DrahtBot added the label Needs rebase on Jan 29, 2019

DrahtBot commented at 3:39 PM on January 29, 2019: member

Needs rebase

fanquake added the label Up for grabs on Jun 17, 2019

fanquake closed this on Jun 17, 2019

MarcoFalke removed the label Up for grabs on Jun 17, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

laanwj referenced this in commit 712b7d9b47 on Feb 5, 2020

sidhujag referenced this in commit 7ff25466b4 on Feb 9, 2020

MarcoFalke referenced this in commit 263f53e2d0 on Feb 17, 2020

sidhujag referenced this in commit 1cc11217e9 on Nov 10, 2020

Munkybooty referenced this in commit 9449010b93 on Jul 29, 2021

Munkybooty referenced this in commit 5cf0ca25d3 on Aug 3, 2021

Munkybooty referenced this in commit cb2d12464b on Aug 5, 2021

Munkybooty referenced this in commit fdc859037d on Aug 8, 2021

Munkybooty referenced this in commit 088a3cab0a on Aug 9, 2021

Munkybooty referenced this in commit 0fd490ff28 on Aug 11, 2021

5tefan referenced this in commit eefcb5fb62 on Aug 12, 2021

DrahtBot locked this on Dec 16, 2021