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

Support for named parameters #4

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mapuo
Copy link

@mapuo mapuo commented Feb 17, 2014

We should have support for named parameters when making JSON RPC calls, this patch makes it possible to do just that with minimal modifications.
When the user asks for client.foo(1) or client.bar(1,2) we should make an array, so the resulting params key should have [1] or [1,2] respectively.
When the user asks for client.foo(:bar => 'foo') we should pass them directly, so the resulting params key should be {"bar": "foo"}.

@marjaimate
Copy link

@mapuo +1

@fxposter
Copy link
Owner

I don't like the solution. If the first argument is JSON object - you will not be able to pass it at all.

@mapuo
Copy link
Author

mapuo commented Feb 19, 2014

What is the exact case? Can you provide an example? :)

@fxposter
Copy link
Owner

client.foo(:some => { :nested => :hash }) - this is the call of method foo with the first parameter { :some => { :nested => :hash } }

@mapuo
Copy link
Author

mapuo commented Feb 19, 2014

I've added a test for that case and it is passing, is this ok with you? :)

@fxposter
Copy link
Owner

No, I mean that my hash is not named params, but just the first parameter of the method. In your implementation we cannot do this anymore.

@mapuo
Copy link
Author

mapuo commented Feb 19, 2014

Agrh! You are right, of course, but that don't leaves us with a lot of choices.
What I was thinking about is will it be better to make the user decide in which mode the parameters are sent to the endpoint. I mean something like this: JSONRPC::Client.new('http://example.com', :named_params => true) and that way for that particular endpoint we know to communicate with named params and not positional. Will leave the option false by default for compatibility.
Can we do that?

@mapuo
Copy link
Author

mapuo commented Feb 20, 2014

Is that refactoring better now in your opinion?

@mapuo
Copy link
Author

mapuo commented Apr 1, 2014

Hey, I was wondering did you had time to see the latest refactoring and will you merge them?
And if not how can I change it to make it better?

@@ -91,7 +104,11 @@ def initialize(url, opts = {})

def method_missing(sym, *args, &block)
if @alive
request = ::JSONRPC::Request.new(sym.to_s, args)
if @helper.named_params? && args.first.is_a?(::Hash)
Copy link

Choose a reason for hiding this comment

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

You need to check for the args.size == 1, because it can be [{foo: 'bar'}, 2]

@mattikus
Copy link

mattikus commented Jul 1, 2016

Hi, I've got an API I'm attempting to use jsonrpc-client for and it requires named parameters and will explicitly reject requests that are nested inside an array. Will there be any further progress on this PR?

@romanbsd
Copy link

romanbsd commented Jul 2, 2016

It's trivial to build an RPC client using faraday and a simple middleware:
https://gist.github.com/romanbsd/f3af3addc88405a55a9c4c577b5525dc

@mattikus
Copy link

mattikus commented Jul 2, 2016

@romanbsd Thank's for the suggestion, that looks perfect for my needs as a very basic client.

@romanbsd
Copy link

romanbsd commented Jul 3, 2016

@mattikus I added a usage example in a comment there

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.

5 participants