RPC client: Simplify command line string-to-JSON-value conversion code #4415

1. 
   jgarzik commented at 11:38 pm on June 25, 2014: contributor

   By default, all command line parameters are converted into JSON string values. There is no need to manually specify the incoming type.

   A binary decision “parse as string or JSON?” is all that’s necessary.

   Convert to a simple class, initialized at runtime startup, which offers a quick lookup to answer “parse as JSON?” conversion question.

   Future parameter conversions need only to indicate the method name and zero-based index of the parameter needing JSON parsing.

2. in src/rpcclient.cpp: in fbed7aa680 outdated

   15@@ -15,92 +16,117 @@
   16 using namespace std;
   17 using namespace json_spirit;
   18 
   19-template<typename T>
   20-void ConvertTo(Value& value, bool fAllowNull=false)
   21+class CRPCConvertParam
   22 {
   23-    if (fAllowNull && value.type() == null_type)
   

   ---

   ---

   

   laanwj commented at 5:34 am on June 26, 2014:

   This removes the fAllowNull check. Not that I think that’s critical, the server will validate the arguments.

   ---

   

   jgarzik commented at 12:19 pm on June 26, 2014:

   Yes, that’s intentional. It’s not needed and adds no value.
3. in src/rpcclient.cpp: in fbed7aa680 outdated

   194+        // insert string value directly
   195+        if (!rpcCvtTable.convert(strMethod, idx))
   196+            params.push_back(strVal);
   197+
   198+        // parse string as JSON, insert bool/number/object/etc. value
   199+        else {
   

   ---

   ---

   

   laanwj commented at 5:43 am on June 26, 2014:

   Nit: Either don’t add an empty line before the else, or surround the first part with braces as well, it looks like there is no else clause and the if(...) ...; is separate while reading this code quickly.

   ---

   

   Diapolo commented at 12:12 pm on June 26, 2014:

   Agreed, this is pretty ugly that way…
4. 
   gavinandresen commented at 3:51 pm on June 26, 2014: contributor

   ACK because ‘better is better.’

   Nit: this could probably be even simpler using boost::assign::insert and assigning into a multimap<string method, int paramIdx>. See http://www.boost.org/doc/libs/1_55_0b1/libs/assign/doc/index.html

5. 
   laanwj commented at 4:04 pm on June 26, 2014: member

   Yes, ACK after code formatting nit.
6. RPC client: Simplify command line string-to-JSON-value conversion code

   By default, all command line parameters are converted into JSON string
   values.  There is no need to manually specify the incoming type.
   
   A binary decision "parse as string or JSON?" is all that's necessary.
   
   Convert to a simple class, initialized at runtime startup, which offers
   a quick lookup to answer "parse as JSON?" conversion question.
   
   Future parameter conversions need only to indicate the method name
   and zero-based index of the parameter needing JSON parsing.

   e35b37b1b8
7. 
   jgarzik commented at 2:42 am on June 27, 2014: contributor

   Rebased + Updated to add braces.
8. 
   BitcoinPullTester commented at 2:53 am on June 27, 2014: none

   Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4415_e35b37b1b8252c3d3f66001ba1efccd263307add/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
9. laanwj merged this on Jun 30, 2014
10. laanwj closed this on Jun 30, 2014

    ---

11. laanwj referenced this in commit 5c184cb850 on Jun 30, 2014
12. in src/rpcclient.cpp: in e35b37b1b8

    195+
    196+        // insert string value directly
    197+        if (!rpcCvtTable.convert(strMethod, idx)) {
    198+            params.push_back(strVal);
    199+        }
    200+
    

    ---

    ---

    

    Diapolo commented at 2:24 pm on June 30, 2014:

    @jgarzik Let me ask this time :), is there still an open pull, which changes this to something more readable in the near future?

    ---

    

    jgarzik commented at 11:41 pm on July 13, 2014:

    @Diapolo Feel free to submit a cleanup PR if you desire! Thanks for asking.
13. jgarzik deleted the branch on Aug 24, 2014
14. MarcoFalke locked this on Sep 8, 2021

Contributors
jgarzik laanwj Diapolo gavinandresen BitcoinPullTester