Page MenuHomePhabricator

Improving getblock by replacing verbose with verbosity
AbandonedPublic

Authored by Mengerian on Jun 28 2018, 15:55.

Details

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

Improving getblock method with more verbosity.
This is really useful when the requester need the whole block.

Test Plan

Locally tested

Diff Detail

Repository
rABC Bitcoin ABC
Branch
rpcExtend
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 2771
Build 3652: Bitcoin ABC Buildbot (legacy)
Build 3651: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Jun 28 2018, 15:55
jasonbcox requested changes to this revision.Jun 28 2018, 21:53
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/rpc/blockchain.cpp
758

Need to add some notes in doc/release-notes.md to indicate that getblock verbose param has been deprecated in favor of verbosity.

771

Conceptually this change is ok, but changing verbose -> verbosity is backwards incompatible. It's probably worth adding verbose back, marking it as (deprecated), and if it has an explicit value set, use verbose's value but only if verbosity isn't set (verbosity should take precedence).

This revision now requires changes to proceed.Jun 28 2018, 21:53

Also, please change the diff title to something like Improving getblock by replacing verbose with verbosity to make it more descriptive.

iFA88 retitled this revision from Improving RPC method to Improving getblock by replacing verbose with verbosity.Jun 29 2018, 06:37

Requested fixes

jasonbcox requested changes to this revision.Jun 29 2018, 16:47
jasonbcox added inline comments.
src/rpc/blockchain.cpp
771 ↗(On Diff #4168)

I don't think it makes sense to have mixed type inputs. I think what you had before (verbosity for numeric) made more sense. However, keeping the verbose (boolean) param will be necessary for backwards compatibility. See my previous comment for a suggestion on implementation.

822 ↗(On Diff #4168)

Looks like you broke something. The rest of the diff is missing?

This revision now requires changes to proceed.Jun 29 2018, 16:47
iFA88 marked 2 inline comments as done.Jun 29 2018, 17:23
iFA88 added inline comments.
src/rpc/blockchain.cpp
771 ↗(On Diff #4168)

I don't think so. If this not good enough for you, please edit it or post me back if this tool don't allow to edit. For backward compatibility this is perfect.

822 ↗(On Diff #4168)

IDK where is it, but the rest are already changed in this file. I think this tool shows now the diff of the last commit.

deadalnix requested changes to this revision.Jun 30 2018, 13:58
deadalnix added a subscriber: deadalnix.

This seems to only change the text of some doc. What's the goal here ?

Also, "tested locally" is not the kind of test plan anyone can reproduce.

Mengerian abandoned this revision.
Mengerian added a reviewer: iFA88.
Mengerian marked 2 inline comments as done.