Page MenuHomePhabricator

fix confusing parameter information
ClosedPublic

Authored by jasonbcox on May 24 2018, 10:15.

Details

Reviewers
deadalnix
qshuai
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rSTAGING26a650b5f5aa: fix confusing parameter information
rABC26a650b5f5aa: fix confusing parameter information
Summary

reference to #10710

Test Plan

// Add a bitcoin-abc node firstly
bitcoin-cli addnode 67.82.89.67 add

// Commands and returned results as following:

bitcoin-cli getaddednodeinfo true

   error code: -24
   error message:
   Error: Node has not been added.
bitcoin-cli getaddednodeinfo true 67.82.89.67

  error code: -1
  error message:
  getaddednodeinfo ( "node" )

// the correct usage:
bitcoin-cli getaddednodeinfo
bitcoin-cli getaddednodeinfo 67.82.89.67

So getaddednodeinfo only have one optional argument<node>.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D1447
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3328
Build 4739: Bitcoin ABC Buildbot (legacy)
Build 4738: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.May 24 2018, 10:15
deadalnix requested changes to this revision.May 24 2018, 13:36
deadalnix added a subscriber: deadalnix.

Please explain what was confusing in the existing example.
Also please fill out a test plan that someone can follow.

This revision now requires changes to proceed.May 24 2018, 13:36

Please explain what was confusing in the existing example.
Also please fill out a test plan that someone can follow.

@deadalnix Have to admit, I am a newer for using arc to contribute. With the same change, I built and ran all unit testings pass. But I can not fix this built error with my several tries.

Explain:
getaddednodeinfo rpc command requires only one parameter<ip optional>. But the help information declares that it works with two parameter<additional bool parameter>. The bitcoin core developers has fix it, view

bitcoin-cli addnode 67.82.89.67 add

bitcoin-cli getaddednodeinfo true
error code: -24
error message:
Error: Node has not been added.

bitcoin-cli getaddednodeinfo true 67.82.89.67
error code: -1
error message:
getaddednodeinfo ( "node" )

// the correct usage:
bitcoin-cli getaddednodeinfo
bitcoin-cli getaddednodeinfo 67.82.89.67
jasonbcox requested changes to this revision.Jun 5 2018, 21:06
jasonbcox added a subscriber: jasonbcox.

I was a little puzzled until I realized that the help text does not specify the param at all. Looking at the implementation, it appears that the bool param is not supported.

I looked at the link you provided and it looks like you're effectively backporting part of this PR: https://github.com/bitcoin/bitcoin/pull/10710
In that case, I think the code block in your latest comment would qualify as the test plan. :)

Things that you need to do before we accept this diff:

  1. Update the Test Plan with the code block you provided showing the tested commands and their outputs.
  2. Update the diff summary to reference partial-backport of PR 10710.
qshuai retitled this revision from fix confusing parameter information to fix confusing parameter information, reference to [#10710](https://github.com/bitcoin/bitcoin/pull/10710).Jun 22 2018, 14:00
qshuai edited the test plan for this revision. (Show Details)
qshuai retitled this revision from fix confusing parameter information, reference to [#10710](https://github.com/bitcoin/bitcoin/pull/10710) to fix confusing parameter information.Jun 22 2018, 14:09
qshuai edited the summary of this revision. (Show Details)

This pr needs more discussion?

qshuai requested review of this revision.Sep 10 2018, 12:19
This revision is now accepted and ready to land.Sep 10 2018, 12:57
jasonbcox edited reviewers, added: qshuai; removed: jasonbcox.
This revision was automatically updated to reflect the committed changes.