Why do some methods accept args rather than everything in a hash?
jackcallister opened this issue · 26 comments
To create some services (postcards for example) the name and address_id are accepted as regular args rather than inside a hash.
postcard = @lob.postcards.create(
name,
address_id,
message: message
)
I'd like to say:
postcard = @lob.postcards.create(
name: name,
address_id: address_id,
message: message
)
It looks clearer and more concise.
def create(options = {})
if options[:to] && !options[:to].is_a?(String)
options[:to] = @resource.format_address_params(options[:to])
end
if options[:from] && !options[:from].is_a?(String)
options[:from] = @resource.format_address_params(options[:from])
end
Lob.submit :post, postcard_url, options
end
I'd be happy to make this change across the board, let me know what you think.
The named args are required args. The hash args are optional args. I wanted to make sure that you are sending valid params.
Although it's possible to have a simple require_params() helper and check for required args, the named args are more explicit. Just opinionated though.
For the postcards API, the docs must have changed since writing this gem, or I must have made a mistake making name a required arg.
If more people want this changed...we are happy to change that. Will use this issue as a vote.
I'll start by voting for all-hash args. That way minor API changes or minor new features won't affect the wrapper.
+1 from me.
+1
+1 for all hash args, also the documentation is currently really misleading and means Ruby developers can't submit postcards without doing a deep dive with the code, so I'd recommend fixing one or the other sooner rather than later =)
@zachfeldman what part of the documentation in particular do you find misleading? Just wondering so the PR to handle all hash args can also handle any documentation vagueness.
@bsiddiqui line 250 of README.md specifies to use a hash to create a new postcard. I attempted to do so and got the "expect 2..3 arguments, supplied 1" error. I went to look inside the library and it turns out that's not how the postcard creation method works at all...it actually works by taking a to, a from, and a hash of other options...which doesn't really make much sense at all. I would never have known this if I didn't look at the library code itself.
@zachfeldman Can you post the code you tried (the one that gave you the error)? I'll look into it for you.
address is a valid address hash.
from value has been changed to protect an address
message is the message from the form i'm submitting
front is a pdf file hosted online.
I'm using Rails 3.2 (not that it really matters here) and I've defined my API key in an initialization file, able to make other requests from the Rails console.
@lob = Lob()
@lob.postcards.create( to: address, from: "adr_cc0c93939234329423", message: params[:final_message], front: 'x.pdf' )
@zachfeldman You are not passing the first argument.
Here's an example from the readme:
@lob.postcards.create(
"John Joe",
"to-address-id",
message: front: File.read("/path/to/file.pdf")
)In your case, you have passed address as params, which will create a new address. That is fine. Here's the fixed piece of your code:
@lob.postcards.create(
"John Doe",
to: address,
from: "adr_cc0c93939234329423",
message: params[:final_message],
front: 'x.pdf'
)Notice I'm passing "John Doe" as the person to send it to. Let me know if this works.
I know, I figured that out, you told me to post the code that wasn't working. I have working code now. My point is that that's really confusing - it looks a lot like it's just a hash of options at first glance.
Overall, the idea of taking some options as arguments and some in a hash just makes no sense to me, especially if you say that you're trying to achieve an ActiveRecord-like syntax - if you were trying to do that, then you would just take a hash of attributes.
@zachfeldman In the current version, required params are taken as mandatory arguments as far as possible for most API methods. The options are mostly for optional params (again as far as possible).
I said as far as possible because some API calls require either one of the params to be present, so for those we let the Lob server decide the required param.
Hey everyone, this thread is recommended only for voting. Please create separate issues for anything else.
Ok...well then this is a difference of opinion. But thanks for writing it anyway, obviously really useful to access the Lob API. I just think it's a confusing way to do it.
If you're looking at the API documentation you think, oh great let me just call this create method and supply it with the necessary options. Not, oh let me think about which order the 'required' options are in as arguments...
@zachfeldman I have to agree on that. That's why the documentation and examples in the readme.
APIs are opinionated. Else we'll just be writing a class that wraps rest-client.
Haha true, true. I totally see where you're coming from. Then I think you should close this issue, right?
@zachfeldman No we'll keep this open for voting and any feedback. My choices were for the initial version of the API.
So whenever this gem goes thru a revamp, whatever changes comes next in terms of args passing, will be based on this thread I suppose.
@HashNuke I'd be keen to contribute this change, unless you guys would rather keep it internal or would like to wait for more feed back?
@jarsbe do you have it on a fork?
@zachfeldman No - I didn't want to invest any time upfront without checking it'd be worthwhile first.
From the vote it seems like the majority is in favor of this change
\cc @bavidar
+1 I'm fine with changing it to all-hash args too. This also matches a few of the other wrappers we have as well and reduces the chance of minor updates to the API affecting the wrapper.