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 help helper class #14502

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
8 participants
@karel-3d
Copy link
Contributor

commented Oct 17, 2018

Inspired by some discussions about RPC documentations ( #14399, #14378 ) I have decided to create a RPC help helper class, that automatically formats the RPC help.

This should be helpful in few respects:

  • it makes the code cleaner
  • it formats the help automatically
  • in the future (I am leaving this for a follow-up PR), it can allow to export JSON, which will allow to make nicer HTML documentation :)

The new classes are in rpc/doc.cpp and they are added in the first commit

The second commit is the actual doc rewrite. The process of rewriting current docs to what is in the PR was semi-automatic - unfortunately, not automatic enough to make it a scripted diff. What I did:

  • wrote a parser of the current RPC documentation
  • made some changes to the RPC help to be parseable
  • generated the code in the PR from that
  • manually cleaned up the code on a lot of places

It's still a huge chunk of code, even when it is just RPC help.

(Third commit is then removing the unused helper function.)

I think it does the same what @achow101 has been trying (by adding what I have in RPCDoc to UniValue instead) here https://github.com/achow101/bitcoin/tree/rpc-help-univ ; I did not use that because I did not fully understand how it was supposed to be working

@karel-3d karel-3d force-pushed the karel-3d:rpc_helper_class branch 2 times, most recently Oct 17, 2018

@karel-3d karel-3d changed the title Rpc helper class Rpc help helper class Oct 17, 2018

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

This is a nice change, and I think making documentation more structured will make it easier to maintain.

I do think it'd be good to replace Table/Row methods with explicit Params / Result methods that make it possible to extract names, types, and structure of parameters. This could be used to generate richer documentation, and support other applications like autocomplete.

@karel-3d karel-3d force-pushed the karel-3d:rpc_helper_class branch 2 times, most recently Oct 17, 2018

@karel-3d

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

That's a good point.

I used "Table" and "Row", since there are few more different table types, and also "Arguments" and "Results" tables are basically the same thing (Arguments have the numbers next to some rows, but only some of them)

The possible table types, by just grepping the source code:

  • Arguments
  • Examples of output descriptors
  • Modes
  • Result
  • Result with verbosity classifier
    • Result (for verbose = false)
    • Result (for verbose = true)
    • Result (for verbose=false)
    • Result (for verbosity = 0)
    • Result (for verbosity = 1)
    • Result (for verbosity = 2)
    • Result (if verbose is not set or set to false)
    • Result (if verbose is set to true)
    • Result: (for verbose = false)
    • Result: (for verbose = true)
  • Result with mode clasifier
    • Result (mode "mallocinfo")
    • Result (mode "stats")

All the Result subtypes are.... yeah basically the same thing, could be treated in some way.

@karel-3d

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

Of course another level would be to somehow match the actual RPC arguments to the arguments in the table, and to the arguments in the short example in the first line, but that is ... just too complex. :)

Good first step would be to replace Table("Arguments") with just Arguments(), the rows with numbered arguments replace with Argument(), add Result("classifier") as a thing, and ... yeah the rest of the tables could stay as Table

@karel-3d

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

The fuctional tests seem to be failing for some reason. I will investigate if I get more general concept ACKs :)

Finally fixed

@karel-3d karel-3d force-pushed the karel-3d:rpc_helper_class branch Oct 17, 2018

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

Concept ACK. The functional tests do seem to be passing for me when I try them locally.

@karel-3d karel-3d force-pushed the karel-3d:rpc_helper_class branch 2 times, most recently Oct 17, 2018

@meshcollider

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

Concept ACK

@kallewoof
Copy link
Member

left a comment

Concept ACK

src/rpc/mining.cpp Outdated
+ HelpExampleRpc("getmininginfo", "")
);

throw RPCDoc("getmininginfo", "")

This comment has been minimized.

Copy link
@kallewoof

kallewoof Oct 18, 2018

Member

Why do you need the , "" part? It looks like it does not require it:

https://github.com/bitcoin/bitcoin/blob/e60061c524f14df8b9e856ec0b89eb6805baf824/src/rpc/doc.h#L70-L71

(this applies to multiple files/places)

This comment has been minimized.

Copy link
@karel-3d

karel-3d Oct 18, 2018

Author Contributor

I just forgot I made the constructor with just one argument. :)

src/rpc/doc.cpp Outdated

std::string RPCDocTable::AsText() const
{
std::string res = "";

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 18, 2018

Member

The initialisation here is redundant.

src/rpc/doc.cpp Outdated

size_t prefixLen = PrefixLength();
for (auto const& row : m_rows) {
std::string code = row.Code();

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 18, 2018

Member

Could be const reference?

src/rpc/doc.cpp Outdated

RPCDoc& RPCDoc::Table(const std::string& name)
{
m_tables.push_back(RPCDocTable(name));

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 18, 2018

Member

emplace_back instead?

This comment has been minimized.

Copy link
@karel-3d

karel-3d Oct 19, 2018

Author Contributor

I have to admit, I don't really understand the difference between emplace_back and push_back

What good does it bring here?

This comment has been minimized.

Copy link
@karel-3d

karel-3d Oct 19, 2018

Author Contributor

...but online wisdom seems to be that emplace_back is faster, because it doesn't create temporary objects, so... ok :)

src/rpc/doc.cpp Outdated

RPCDoc& RPCDoc::Example(const std::string& code)
{
m_examples.push_back(RPCDocExample(code));

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 18, 2018

Member

emplace_back instead?

src/rpc/doc.cpp Outdated

RPCDoc& RPCDoc::Example(const std::string& code, const std::string& example)
{
m_examples.push_back(RPCDocExample(code, example));

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 18, 2018

Member

emplace_back instead?

src/rpc/doc.cpp Outdated

RPCDoc& RPCDoc::ExampleCli(const std::string& description, const std::string& methodName, const std::string& args)
{
m_examples.push_back(

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 18, 2018

Member

emplace_back instead?

src/rpc/doc.cpp Outdated

RPCDoc& RPCDoc::ExampleRpc(const std::string& description, const std::string& methodName, const std::string& args)
{
m_examples.push_back(RPCDocExample(

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 18, 2018

Member

emplace_back instead?

"\nExamples:\n" +
HelpExampleCli("submitheader", "\"aabbcc\"") +
HelpExampleRpc("submitheader", "\"aabbcc\""));
throw RPCDoc("submitheader", "\"hexdata\"")

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 18, 2018

Member

RPCDoc should be derived from std::exception?

This comment has been minimized.

Copy link
@karel-3d

karel-3d Oct 19, 2018

Author Contributor

Hm, not sure if I like it... I like how explicit it is now, so you can either have a RPCDoc object (which could later be used somewhere else too) and then explicitly make exception out of it

Also I think that right now when you throw different exception than runtime_error, bitcoind crashes instead of bitcoin-cli writing out the help. I will look once more to be sure

src/rpc/doc.h Outdated

public:
RPCDocExample(const std::string& description, const std::string& code);
RPCDocExample(const std::string& code);

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 18, 2018

Member

Should be explicit?

src/rpc/doc.h Outdated
const std::string m_description;

public:
RPCDocTableRow(const std::string& code);

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 18, 2018

Member

Should be explicit?

src/rpc/doc.h Outdated
size_t PrefixLength() const;

public:
RPCDocTable(const std::string& name);

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 18, 2018

Member

Should be explicit?

src/rpc/doc.h Outdated
std::vector<RPCDocExample> m_examples;

public:
RPCDoc(std::string methodName);

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 18, 2018

Member

Should be explicit?

src/rpc/doc.cpp Outdated
return res;
}

RPCDoc::RPCDoc(std::string methodName, std::string firstArguments)

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 18, 2018

Member

Should be const references?

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

Concept ACK. However, this seems to be a large change that is probably impossible to review without splitting it up in smaller chunks and/or making it a scripted diff.

While the change seems to touch every help text, it doesn't yet make it meaningfully easier to actually produce the help text machine-generated. You split out the rpc method name, but then pass all arguments as a single string and then each of them again as a "Row". This doens't really solve the issue that the documentation is inconsistent in itself (Starting with the white space and formatting of the single line arg string [cf. #14459] and going on to just forgetting the single line arg string [as in importprunedfunds], ...)

Ideally we'd either merge some exhaustive fix similar to #14459 and then a pure scripted-diff to make it auto-generated (and check that the resulting documentation does not differ after the scripted-diff) or we do the auto-generation without the scripted diff and check that the resulting diff in the documentation looks similar to #14459.

I have quickly hacked up a first step of how I think here: #14530

@karel-3d

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2018

Thanks for feedback.

ad script-diff: I can probably make it script-diffed, it will need some constistency fixed first so the text is parseable, but that can be done as a separate commit. (It is mostly whitespace, missing : after "Arguments" etc)

ad other notes:

Yes, this PR lets most of the arguments/"rows" be as-is, as the various styles seemed too different to me to make consistent, so I gave up and just let it be; I focused mainly on getting rid of the "space-formatting" and making it automatically consistent.

If the goal is indeed to make generation of, for example, the first-line argument etc more automatic, that can be done, but will take more time :) but sure, it will be a nicer code

If I got it correctly. Let's say, walletfundedpsbt

https://bitcoincore.org/en/doc/0.17.0/rpc/wallet/walletcreatefundedpsbt/

The current RPC, at least start, is

walletcreatefundedpsbt [{"txid":"id","vout":n},...] [{"address":amount},{"data":"hex"},...] ( locktime ) ( replaceable ) ( options bip32derivs )

Creates and funds a transaction in the Partially Signed Transaction format. Inputs will be added if supplied inputs are not enough
Implements the Creator and Updater roles.

Arguments:
1. "inputs"                (array, required) A json array of json objects
     [
       {
         "txid":"id",      (string, required) The transaction id
         "vout":n,         (numeric, required) The output number
         "sequence":n      (numeric, optional) The sequence number
       } 
       ,...
     ]
2. "outputs"               (array, required) a json array with outputs (key-value pairs)
   [
    {
      "address": x.xxx,    (obj, optional) A key-value pair. The key (string) is the bitcoin address, the value (float or string) is the amount in BTC
    },
    {
      "data": "hex"        (obj, optional) A key-value pair. The key must be "data", the value is hex encoded data
    }
    ,...                     More key-value pairs of the above form. For compatibility reasons, a dictionary, which holds the key-value pairs directly, is also
                             accepted as second parameter.
   ]

Should the "subcodes" in inputs/outputs arguments, and their help, be also generated from some object (that seems to be m_inner in your example)? Including all the ",...." ? Should the first line stay like it is, or should it instead be walletcreatefundedpsbt inputs outputs ( locktime ) ( replaceable ) ( options bip32derivs )?

I am just saying it will be pretty hard to do this, that's why I opted for "just" adding Row to each table (which also took care of all the Results, that have the same formatting logic). :)

but if the goal is indeed to make all the doc even more uniform, it's probably better than this "naive" approach that just formats the spaces

edit: I look more closely at your #14530 and it indeed produces all the "pseudo-objects" on the left. OK

@karel-3d

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2018

#14530 looks great, I will try to work on top of that

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

Needs rebase
@karel-3d

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2018

Closing this, in support of approach similar to #14530

It will be harder to rewrite current source code to that, but ultimately will be more helpful.

@karel-3d karel-3d closed this Oct 24, 2018

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.