Page MenuHomePhabricator

Reword confusing warning message in RPC linter
ClosedPublic

Authored by jasonbcox on Apr 2 2020, 19:25.

Details

Summary

It's easy to come across this message by simply forgetting to add an entry
in src/rpc/client.cpp when an argument name matches an existing entry in that table.

This patch clarifies both the intent and reason for the warning, and suggests possible fixes.

Test Plan

Add this line to commands in src/rpc/avalanche.cpp:
{ "avalanche", "addavalanchepeer", addavalanchepeer, {"nodeid"}},
arc lint src/rpc/avalance.cpp will fail with the warning message (note "nodeid" conflicts with disconnectnode's "nodeid")

Add this line to the table in src/rpc/client.cpp:
{"addavalanchepeer", 0, "nodeid"},
arc lint src/rpc/avalance.cpp passes

Example output:

>>> Lint for /home/jasonbcox/projects/bitcoin-abc:


   Warning  (RPC_MAPPING_WARNING) RPC mapping warning
    In order to keep a consistent API, arguments of the same name are
    expected to either both be string-typed or converted from JSON. But there
    was a conversion mismatch: ["'getavalanchekey' has argument 'nodeid' of
    type 'string'", "'disconnectnode' has argument 'nodeid' of type 'JSON'"].
    Common root causes for this warning: 1) The command and/or argument are
    missing from the conversion table in 'src/rpc/client.cpp'. 2) Arguments
    of the same name are being converted from JSON for some commands, but not
    for others. Consider renaming arguments such that one name is used for
    strings and the other for conversions from JSON.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
rpc-lint
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 10106
Build 18042: Default Diff Build & Tests
Build 18041: arc lint + arc unit

Event Timeline

Reworded some more according to offline feedback from Fabien

deadalnix requested changes to this revision.Apr 2 2020, 21:58
deadalnix added a subscriber: deadalnix.

([('disconnectnode', True), ('getavalanchekey', False)])

This should not be part of the error message. Would something like disconnectnode does X, while getavalanchekey does Y. really be too much to ask for? Also a mention of rpc/client.cpp somewhere so one doesn't need a phd is archeology to figure out what's up?

This revision now requires changes to proceed.Apr 2 2020, 21:58

([('disconnectnode', True), ('getavalanchekey', False)])

This should not be part of the error message. Would something like disconnectnode does X, while getavalanchekey does Y. really be too much to ask for? Also a mention of rpc/client.cpp somewhere so one doesn't need a phd is archeology to figure out what's up?

I can fix that up some more, ya. The true/false thing is stupid.

See the example message in the test plan. It indeed names rpc/client.cpp as you said.

Removed cryptic ('command', 'bool') tuples in favor of a formatted list

This revision is now accepted and ready to land.Apr 3 2020, 00:18