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

docs: update body param type for run_request #1545

Merged
merged 4 commits into from
Jan 20, 2024
Merged

docs: update body param type for run_request #1545

merged 4 commits into from
Jan 20, 2024

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Dec 31, 2023

Description

It looks like this was already picked up in the past (via #601) but the docs have since been overhauled and this was lost in the transition.

I've used Object as my understanding is really body doesn't care what it's passed so long as it can be turned into a string or JSON, and then beyond supporting calling methods like to_json, that's up the object being passed in to handle.

Todos

List any remaining work that needs to be done, i.e:

  • Tests
  • Documentation

@olleolleolle
Copy link
Member

Another way to communicate the parameter type is to offer which method it ought to support. That's an alternative. If we do use Object, we may extend the description string in the docblock, saying what the Object may be, what is required of it.

@G-Rath
Copy link
Contributor Author

G-Rath commented Jan 4, 2024

I'm not sure how useful or possible that would be since it's really up to the middleware right? When I originally opened this I realised shortly after this that this method isn't quite what I was wanting to use and that the JSON handling is actually done by a middleware rather than the core which I think means this change is less important (though not incorrect) because it means it's not really something the core can document 🤷

@iMacTia
Copy link
Member

iMacTia commented Jan 11, 2024

which I think means this change is less important (though not incorrect) because it means it's not really something the core can document

That's a fair point, I can totally see the confusion here.
The current String is clearly too limited, at the same time though I agree with @olleolleolle that Object is too broad and hardly adds any value to the documentation.

So here is my proposal: let's use [String, Hash, Array, nil] and improve the doc comment a bit to point out that this is based on the default configuration, and that using different middleware will potentially allow different types to be supported.

This is because by default Faraday uses the url_encoded middleware, that supports all those types:

# The following all run a request with body: "a=1"
Faraday.post('http://google.com', 'a=1')
Faraday.post('http://google.com', {a: 1})
Faraday.post('http://google.com', [[:a,1]])

lib/faraday/connection.rb Outdated Show resolved Hide resolved
@iMacTia iMacTia merged commit 18154c8 into lostisland:main Jan 20, 2024
1 of 2 checks passed
@G-Rath G-Rath deleted the patch-1 branch January 21, 2024 19:02
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.

3 participants