Improving getblock by replacing verbose with verbosity
AbandonedPublic
Actions

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

Details

Reviewers

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

Repository

rABC Bitcoin ABC

Branch

rpcExtend

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 2774
Build 3658: Bitcoin ABC Buildbot (legacy)
Build 3657: arc lint + arc unit

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 ↗	(On Diff #4165)	Need to add some notes in doc/release-notes.md to indicate that getblock verbose param has been deprecated in favor of verbosity.
771 ↗	(On Diff #4165)	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	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	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	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	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.