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

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

Details

Reviewers

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:

getrpchelps.sh253 BDownload

checkrpc.sh678 BDownload

RPC Help diff:

diffRPCs31 KBDownload

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

nakihito created this revision.Mar 26 2020, 00:02

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

Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 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.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

nakihito planned changes to this revision.Mar 26 2020, 00:04

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B9979: Diff 17186.Mar 26 2020, 00:13

nakihito requested review of this revision.Mar 26 2020, 00:35

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

deadalnix added a comment.Mar 26 2020, 01:51

This comment was removed by deadalnix.

deadalnix added inline comments.Mar 26 2020, 01:53

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 planned changes to this revision.Mar 26 2020, 02:05

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.

Rebased.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B9993: Diff 17217.Mar 26 2020, 18:03

Rebased.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

nakihito added a child revision: D5585: Merge #14987: RPCHelpMan: Pass through Result and Examples.Mar 26 2020, 23:53

Harbormaster completed remote builds in B10002: Diff 17230.Mar 27 2020, 00:01

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

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

nakihito edited the test plan for this revision. (Show Details)Mar 30 2020, 20:46

Fixed some discrepancies and updated test plan.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

nakihito edited the test plan for this revision. (Show Details)Mar 30 2020, 20:50

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10045: Diff 17294.Mar 30 2020, 20:57

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

Updated test plan as requested.

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.