Add support for JSON RPC named parameters #33

pedrobranco · 2017-09-11T23:51:18Z

This PR adds support for JSON RPC named parameters (only available in 0.14.x).

const balance = await new Client(config.bitcoind).getBalance({
  account: '*',
  minconf: 0
});

// Same as:
const balance = await new Client(config.bitcoind).getBalance('*', 0);

ruimarinho · 2017-10-09T23:57:40Z

src/index.js

@@ -84,6 +84,8 @@ class Client {
        .value();
    }

+    this.hasNamedParametersSupport = semver.satisfies(version || '0.14.0', '>=0.14.0');


Here, it is assumed that 0.14.0 is the default version as per this condition. I would rather do this globally - affecting available methods as well - or only enable this when a version is set and matches the given constraints. My vote is on the latter.

We could enable this feature by default, and only disable it when version is passed and is lower than 0.14.0 just like all methods. WDYT?

ruimarinho · 2017-10-09T23:58:03Z

src/index.js

@@ -113,6 +115,11 @@ class Client {
      parameters = _.dropRight(parameters);
    }

+    // Support named parameters (Bitcoin Core >=0.14.0 only).


This comment should be sufficient on the boolean setter.

ruimarinho · 2017-10-10T00:04:06Z

test/index_test.js

@@ -242,6 +251,16 @@ describe('Client', () => {

        transactions.should.be.an.Array().and.empty();
      });
+
+      it('should support named parameters', async () => {


Is there a more generic test we can add that doesn't require adding a should support named parameters type of test for each and every method?

Since each method has its own named arguments, we cannot add a generic test (or at least without adding all method definitions).

ruimarinho · 2017-10-31T00:35:24Z

src/index.js

@@ -63,6 +63,7 @@ class Client {

    this.agentOptions = agentOptions;
    this.auth = (password || username) && { pass: password, user: username };
+    this.hasNamedParametersSupport = true;


@pedrobranco I don't think the default should be true. Without passing a specific version, we can't be sure of that, so it seems rather imprudent to simply enable this for all clients.

ruimarinho · 2017-10-31T00:59:09Z

Added information to README.

ruimarinho · 2017-10-31T01:06:24Z

@pedrobranco I've got the changes done locally but I'm unable to push to your branch. Can you enable collaborative commit access?

ruimarinho · 2017-10-31T23:00:12Z

src/index.js

@@ -77,6 +78,14 @@ class Client {
    let unsupported = [];

    if (version) {
+      // Convert to semver (removing pathes).
+      if (!/[0-9]+\.[0-9]+\.[0-9]+/.test(version)) {


I don't think we need to run this test as exec gives us both informations (the test and the capture which we need below).

ruimarinho · 2017-10-31T23:13:30Z

src/index.js

+
+      [version] = /[0-9]+\.[0-9]+\.[0-9]+/.exec(version);
+
+      this.hasNamedParametersSupport = semver.satisfies(version, '>=0.14.0');


If version is null, the result will be false, so everything will be ok here.

ruimarinho · 2017-11-25T00:17:04Z

src/index.js

@@ -77,6 +78,14 @@ class Client {
    let unsupported = [];

    if (version) {
+      // Convert to semver (removing pathes).


ruimarinho · 2017-11-25T01:48:14Z

Please follow this PR on #51.

pedrobranco added the enhancement label Sep 11, 2017

pedrobranco self-assigned this Sep 11, 2017

pedrobranco force-pushed the enhancement/support-jsonrpc-named-parameters branch from 5c131dd to d5b14cf Compare September 27, 2017 10:34

pedrobranco requested a review from joaopaulofonseca September 27, 2017 10:34

pedrobranco force-pushed the enhancement/support-jsonrpc-named-parameters branch from d5b14cf to 9fe550d Compare September 27, 2017 14:25

ruimarinho requested changes Oct 10, 2017

View reviewed changes

pedrobranco force-pushed the enhancement/support-jsonrpc-named-parameters branch from 9fe550d to 2a03715 Compare October 30, 2017 12:20

ruimarinho requested changes Oct 31, 2017

View reviewed changes

Support JSON RPC named parameters

7ca5b94

pedrobranco force-pushed the enhancement/support-jsonrpc-named-parameters branch from 2a03715 to 7ca5b94 Compare October 31, 2017 12:18

Fix version bug when passing 0.15.0.1

4f66db2

ruimarinho requested changes Oct 31, 2017

View reviewed changes

ruimarinho reviewed Nov 25, 2017

View reviewed changes

ruimarinho mentioned this pull request Nov 25, 2017

Add support for named parameters when using the JSON-RPC interface #51

Merged

ruimarinho closed this Nov 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for JSON RPC named parameters #33

Add support for JSON RPC named parameters #33

pedrobranco commented Sep 11, 2017

ruimarinho Oct 9, 2017

pedrobranco Oct 30, 2017 •

edited

Loading

ruimarinho Oct 9, 2017

ruimarinho Oct 10, 2017

pedrobranco Oct 30, 2017

ruimarinho Oct 31, 2017

ruimarinho commented Oct 31, 2017

ruimarinho commented Oct 31, 2017

ruimarinho Oct 31, 2017

ruimarinho Oct 31, 2017

ruimarinho Nov 25, 2017

ruimarinho commented Nov 25, 2017


		[version] = /[0-9]+\.[0-9]+\.[0-9]+/.exec(version);

		this.hasNamedParametersSupport = semver.satisfies(version, '>=0.14.0');

Add support for JSON RPC named parameters #33

Add support for JSON RPC named parameters #33

Conversation

pedrobranco commented Sep 11, 2017

Choose a reason for hiding this comment

pedrobranco Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruimarinho commented Oct 31, 2017

ruimarinho commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruimarinho commented Nov 25, 2017

pedrobranco Oct 30, 2017 •

edited

Loading