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
Deletes with bodies #693
Comments
A PR will be up shortly for your review. |
Hi @Carlbc18, could you please provide a link to the above changes? |
https://tools.ietf.org/html/rfc7231#section-4.3.5 FYI: This same block of text applies to GET && HEAD requests as well. specifically this part of the spec:
|
i have a code change with tests that works (not sure if you want it). how do i create a pr for this project? |
this is actually part of HTTP 1.1 |
I would definitely consider it if you want to create a PR. |
Thanks @iMacTia. I'll put it up. Whats your views on changing the version finally have a major version number with this? This change could be breaking for many. I'd like to follow semver pattern, and version faraday to be 1.0.0 at this point. Let me know your thoughts. |
We're definitely trying to stick to semver and we've v1.0 scheduled with some breaking changes. |
@iMacTia i versioned to 0.13.0 and the PR is up. We can take any other conversation around this to the PR as i'm sure there will be comments. Thanks again! |
This is a follow up on my #695 (comment). conn = Faraday.new(...)
conn.get('/path', {page: 1, per: 10})
# performs "GET /path?page=1&per=10" Option 1 as stated in my comment would be to make this behaviour configurable: conn = Faraday.new(..., standard: :http1_1)
conn.get('/path', {page: 1, per: 10})
# performs "GET /path" with body "page=1&per=10" (assuming www_form_url_encoded request) Option 2 instead would mean something more powerful: conn = Faraday.new(...) # note no special config here
conn.get('/path', {page: 1, per: 10}, body: {some_key: 'some_value')
# performs "GET /path?page=1&per=10" with body "some_key=some_value" |
There's an issue though, option 2 might only work with ruby >= 2 as ruby 1.9 doesn't support keyword params 😞 |
I think in regards to this not working on ruby 1.9 I feel like that is ok. Ruby 1.9 is many years old. I'd advocate we version bump to at least 2.0 for minimum ruby version, which would go inline with a v1.0 release of Faraday. Also option 2 more closely follows http 1.1 spec in my opinion as you'd have the option to pass parms and or a optional body. I feel like attempting to maintain backwards compatibility with a http spec feature that does work with ruby 1.9 is going to cause problems. I think this feature would be better suited for V1 of Faraday. I feel like there are too many variables were needing to support for this change (ruby v, params passing). Maybe I'm overthinking.... |
I totally agree with al the above. That's indeed the best course of action, even though this means we'll still have to wait for v1.0 |
So to define a v1.0 backward compatible API, we could attempt the following: GET, HEAD, DELETE REQUESTSconn = Faraday.new(...) # note no special config here
conn.get(path, url_params, body: {...})
# second parameter is URL PARAMS, optionally accept a named parameter for request body
# works the same for head and delete POST, PUT, PATCH REQUESTSconn = Faraday.new(...) # note no special config here
conn.post(path, body, url_params: {...})
# second parameter is REQUEST BODY, optionally accept a named parameter for url_params
# works the same for put and patch I think this way we're covering every possible combination and extending post/put/patch requests as well! |
Definitely agree. Do we want to decline my PR and open a new one for this? Is there a V1 branch cut we can work off of? |
Happy to close that one and start from scratch as it will be completely different. Sorry for that! I'll create a V1 branch before merging in so feel free to start from master and I'll redirect the PR :) |
Hi @Carlbc18 just wanted to let you know that branch |
Thank you, thank you! 😀 |
For anyone still waiting on this, I found the following workaround which seems to work in my case:
I hope this helps someone until delete no longer nils the body by default |
So is there any update on when this will be available? |
I haven't seen any PR yet, but works on v1 are slowly progressing. For a fully backwards compatible solution, the PR can be branched out of master, and I can make another 0.x release. |
#855 is a draft PR to support this. |
I'm rejecting this feature. You can use the block form in Faraday to make a request with a body: conn.delete(url) do |req|
req.body = { some_key: 'some value' }
end This idea would have made simple deletes with bodies a single one liner, but the implementation adds more complexity and surprising behavior than I'd like. This block form works great in Faraday already. I prefer modifying properties on a class to adding more arguments to a method: conn.delete('/foo/1') do |req|
req.headers["X-Men"] = "marvel"
req.params[:confirm] = 1
req.body = { some_key: 'some value' }
end |
HTTP spec has changed in 2014 and allows for deletes to have bodies. Is there a plan in place to resolve your delegator method in connection.rb to allow for a body with the delete method and not nil'ing it out?
The text was updated successfully, but these errors were encountered: