Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added promise support #2

Merged
merged 2 commits into from
Aug 29, 2018
Merged

Added promise support #2

merged 2 commits into from
Aug 29, 2018

Conversation

JBaczuk
Copy link
Contributor

@JBaczuk JBaczuk commented Aug 28, 2018

If you think this is useful, I'd love your feedback.

Copy link
Contributor

@kallewoof kallewoof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool!

Minor style nits: put space after if keyword and before the parens, i.e. if (...), and put semi-colons at the end of lines. I can fix these later if you prefer.

index.js Outdated
this.calb = true;
} else {
this.calb = false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can shrink to

this.calb = typeof args[args.length - 1] === 'function';

index.js Outdated
method: cmd.toLowerCase(),
params: slice(args, 0, args.length - 1),
}, args[args.length - 1]);
params: slice(args, 0, args.length),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for call to slice(), can just use params: args.

index.js Outdated
return callback(e);
}
return callback(bufDeserialized.error, bufDeserialized);
reject(err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When calb is set, both callback() and reject() will be called. Use else for the !this.calb case, or return in the above if block?

index.js Outdated
@@ -136,61 +136,104 @@ function RpcAgent(opts = {}) {
this.user = opts.user || 'user';
this.pass = opts.pass || 'pass';
this.prot = opts.ssl ? https : http;
this.calb = false; // Defaults to promises
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need this line. this.calb is set every time a command is invoked.

index.js Outdated
return callback(new Error('bitcoin JSON-RPC connection rejected: 401 unauthorized'));
}
reject(new Error('bitcoin JSON-RPC connection rejected: 401 unauthorized'));
return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's perfectly fine to return the reject call here and in the other places, I think, to save the extra return lines.

@JBaczuk
Copy link
Contributor Author

JBaczuk commented Aug 29, 2018

Thanks! I agree with all of the changes, I just updated my eslint file to follow those style rules, so it was no big deal. The current branch should reflect all of the changes suggested above.

@JBaczuk
Copy link
Contributor Author

JBaczuk commented Aug 29, 2018

I squashed commits, but would it have been easier for you if I didn't on those latest changes? I haven't done a ton of contributing via github so I'm trying to figure out a good process.

@kallewoof kallewoof merged commit 83b16e5 into dgarage:master Aug 29, 2018
@kallewoof
Copy link
Contributor

Squashing is totally fine. Thanks!

@JBaczuk JBaczuk deleted the add_promises branch August 29, 2018 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants