-
Notifications
You must be signed in to change notification settings - Fork 23
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 Client ability to perform custom GET and POST #31
Conversation
It would be great to have this merged. Is there a chance that anybody take a look at it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Miguelcldn Thanks for your PR!
Please review my comment.
# - url: the URL to request | ||
# | ||
def get(url) | ||
MercadoPago::Request.wrap_get("#{url}?access_token=#{@access_token}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Miguelcldn Why didn't you add payload support here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could, just thought it was not necessary. I will now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etagwerker , why do you think that's needed? wrap_get
does not accept a payload. If you want this method to accept a payload, wrap_get
should be modified as well. In addition, the official MP gem does not support a payload either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that too, wrap_get
doesn't need a payload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etagwerker , Did you have the chance to see the comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Miguelcldn I'm sorry, I didn't mean "payload", I meant "headers".
Sure, it doesn't need a payload, but someone might want to send specific headers.
See:
mercadopago/lib/mercadopago/request.rb
Lines 34 to 36 in 0276edd
def self.wrap_get(path, headers = {}) | |
make_request(:get, path, nil, headers) | |
end |
Also, in Mercado Pago's API doc: https://www.mercadopago.com.ar/developers/en/api-docs/account/movements/
If we are going to add this feature, we should make sure that it's flexible enough to take headers like accept: application/json
Could you change that in the new get
and post
methods? Just add an optional parameter as the last parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done @etagwerker, I had to change the way default headers were set because if I'd set headers as {}
in Client.get
then wrap_get
would not set the default headers, same for post.
Also, now it supports adding custom query strings.
@Miguelcldn Thanks! 👍 |
The official MercadoPago ruby SDK supports generic methods in order to access any other resource not available as a method.
I've seen this feature useful because I needed to create a preapproval plan as a POST to
/preapproval_plan
. Though I could simply add a function to do it, I took the chance to implement generic calls at once.