Page MenuHomePhabricator

Merge #14796: rpc: Pass argument descriptions to RPCHelpMan
AbandonedPublic

Authored by nakihito on Mar 26 2020, 00:02.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

fabca42c68 RPCHelpMan: Add space after colons in extended description (MarcoFalke)
fafd040f73 rpc: Add description to fundrawtransaction vout_index (MarcoFalke)
1db0096f61 rpc: Pass argument descriptions to RPCHelpMan (MarcoFalke)

Pull request description:

This will normalize the type names and formatting for the rpc arguments

Tree-SHA512: 6ab344882f0fed36046ab4636cb2fa5d2479c6aae22666ca9a0d067edbb9eff8de98010ad97c8ce40ab532d15d1ae67120a561b0bf3da837090d7de427679f4f

Backport of Core PR14796

Test Plan

On master:

ninja
./bitcoind
./getrpchelps.sh RPCs master

On the patched branch:

ninja
./bitcoind
./getrpchelps.sh RPCs patched
./checkrpc.sh RPCs diffRPC

Verify the diffs of each RPC.

Scripts:


RPC Help diff:

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR14796
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9979
Build 17806: Default Diff Build & Tests
Build 17805: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Mar 26 2020, 00:02

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those PRs have been inserted into the summary for reference.

nakihito added inline comments.
src/rpc/mining.cpp
385–390

This was mistakenly added in D5571. Looks like an argument that core has that we removed.

deadalnix requested changes to this revision.Mar 26 2020, 01:50
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/rpc/mining.cpp
385–390

Well, D5571 is not in, so there is time to fix it and rebase.

This revision now requires changes to proceed.Mar 26 2020, 01:50
This comment was removed by deadalnix.
src/rpc/mining.cpp
385–390

Wait, you pointed at the wrong diff. In any case, you should fix the error in isolation and then rebase. Packaging an fix with a large refactoring is a perfect way to create a disaster.

nakihito added inline comments.
src/rpc/mining.cpp
385–390

Ah, you're right. I'll do as you suggested. For the record it was supposed to be D5548.

deadalnix requested changes to this revision.Mar 29 2020, 15:31

I spotted at least one discrepancy. This is review hell. Generating the list of help for all RPC before and after that patch and providing a diff is the only way to go here.

src/rpc/blockchain.cpp
233 ↗(On Diff #17230)

default is 0.

This revision now requires changes to proceed.Mar 29 2020, 15:31

Fixed some discrepancies and updated test plan.

deadalnix requested changes to this revision.Mar 30 2020, 23:14

Alright, I looked at a few diffs, but this is clear that this is still not reviewable as this.

First, I'd recommend putting it all in one diff instead of many smaller ones. The process to go through them all is very slow.
Second, there is a lot of whitespace changes in the diffs. You can remove all of them by preprocessing the files to collapse consecutive white spaces into just one space.

But I see you got the scripts and all, so that should be good. Also keep them around and consider pushing that work forward by providing a way to generate online doc for the rpc API for the website, like this: https://bitcoincore.org/en/doc/0.19.0/

This revision now requires changes to proceed.Mar 30 2020, 23:14
nakihito edited the test plan for this revision. (Show Details)
nakihito edited the test plan for this revision. (Show Details)

Updated test plan as requested.

deadalnix requested changes to this revision.Apr 1 2020, 02:45

Ok I spent forever, went through maybe 1/4 of it and find at least one problem.

IMO you need to start reviewing your own diff and question if that is possible at all. I'm giving up. You need to make this easier to review or this will not get in.

src/rpc/mining.cpp
293 ↗(On Diff #17294)

The dummy became mandatory.

402 ↗(On Diff #17294)

This is adding ... at the end, so you need a coma.

This revision now requires changes to proceed.Apr 1 2020, 02:45

D5700 and its tree will break this patch into smaller parts.