Cleanup client HTTP methods

[jacegu]

Following up with the conversation @weppos had in Slack.

[weppos]

@jacegu actually, my proposal is to keep the options, and add the data where necessary.

E.g.

def get(path, options = {})
  # ...

def post(path, data, options = {})
  # ...

[jacegu]

Last commit is the smallest step to make remove the confusion about the unused parameters.

From here we can move forward and have the request body as a separate parameter in the request. If we do that the methods with an actual body would look like this:

def post(path, data, options = {})
  execute :post, path, data, options
end

while the methods without it would look like this:

def get(path, options = {})
  execute :get, path, {}, options
end

We would also have to change how we compose the HTTParty request in the request method as query and headers would no longer be part of the data parameter, but part of a fourth, newly added, one.

Finally we would have to revert all the:

attribute.merge(options)

inside each endpoint method to have data and options as separate parameters where appropriate.

Want to double check first that we want to move forward with this because it will be some work. It may be worth to consider if the change would benefit us when the time to replace HTTParty comes.

Thoughts?

[jacegu]

That's how the client would look like with the change @weppos. Still need to undo all the attributes.merge(options). Build passes though... (Which makes me nervous).

[weppos]

Although the RFC doesn't mention the data payload for a DELETE action, it's common practice to consider DELETE to accept a payload. I would allow it, since I believe we use (or used) it.

[jacegu]

Since we are not accepting attributes in the delete methods we would have to change all delete calls from something like:

response = client.delete(Client.versioned("/%s/contacts/%s" % [account_id, contact_id]), options)

to (note the extra {} in place of attributes):

response = client.delete(Client.versioned("/%s/contacts/%s" % [account_id, contact_id]), {}, options)

Either that, or add the attributes to each delete endpoint switching:

def delete_contact(account_id, contact_id, options = {})

to

def delete_contact(account_id, contact_id, attributes = {}, options = {})

[weppos]

No, we should not add it to the method signature. We should use

response = client.delete(Client.versioned("/%s/contacts/%s" % [account_id, contact_id]), {}, options)

even if a nil value would seem more appropriate

response = client.delete(Client.versioned("/%s/contacts/%s" % [account_id, contact_id]), nil, options)

[weppos]

So far so good, @jacegu. It definitely looks cleaner to me.

[jacegu]

@weppos you can check where this is going. If you think it's ok we can go back to consider switching the default value of every attribute parameter from {} to nil.

[weppos]

@jacegu I'd leave options default to {}, but it makes sense to me to change data default to be nil as any other value could potentially be a valid non-empty value.

[jacegu]

@weppos changing options' default value was never on the table. 👍 to changing data and attributes default value to nil.

[jacegu]

@weppos I just had a thought: maybe when you were talking about options you actually meant attributes... I've stopped here for that reason.

The pending changes before calling this PR done, are doing what I did for the contacts for every other method that takes attributes. Will wait to verify that we are on the same page before moving forward.

[weppos]

@jacegu contacts look good to me. What I'd probably change, is to pass nil in the delete instead of {}. In other words, when the data is empty, we should pass nil and not {}.

I also wonder if it makes sense to set attributes as optional parameters in create/update. It doesn't make sense to create a contact with empty attributes, I'd remove the default value and leave them as mandatory arguments.

Does it make sense?

[jacegu]

What I'd probably change, is to pass nil in the delete instead of {}

Missed that! Should had been changed.

I also wonder if it makes sense to set attributes as optional parameters in create/update. It doesn't make sense to create a contact with empty attributes, I'd remove the default value and leave them as mandatory arguments.

I agree.

Moving forward with this then. 👍

[weppos]

@jacegu I suppose it should be nil here.

[jacegu]

Thanks for spotting that. Couldn't get them all 😅

[weppos]

Thanks @jacegu, it definitely looks cleaner now that attributes and options are separated. Besides a nil value in the get that seems more appropriate to me, this is a for me.