- Notifications
You must be signed in to change notification settings - Fork22.1k
Addas to encode a request as a specific mime type.#21671
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
e8e5f46 to749b74bComparekaspth commentedSep 19, 2015
All right, added some tests and documentation. |
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.
Am I right in making this claim? Definitely feels like it could use some shine, but don't have a fully formed idea of which direction to take 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.
I don't think so. Try to test it in a controller with curl.
arthurnn commentedOct 18, 2015
I like the idea behind this. 👍 I remember having to do those setups before, and indeed we could make that setup easier. |
arthurnn commentedOct 18, 2015
dont forget to add a changelog for 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.
Should probably do a test for how much of an impact this has on our integration test speed.
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.
Can we defer the path parsing toappend_format_to? If we don't need to append a format and if the paths is not a full path there is no need to parsing.
kaspth commentedOct 18, 2015
@arthurnn cool 😁 |
Turns```post articles_path(format: :json), params: { article: { name: 'Ahoy!' } }.to_json, headers: { 'Content-Type' => 'application/json' }```into```post articles_path, params: { article: { name: 'Ahoy!' } }, as: :json```749b74b to52bb2d3CompareThere 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.
@rafaelfranca tried your suggestion to call a custom controller via curl, but couldn't get it to work.
When I byebugrequest_parameters it seems tofetch_header which calls@env.fetch which already seems to have an empty hash for that key. Would you have any ideas why? I'll keep digging 😁
kaspth commentedJan 4, 2016
Reworked this to use Mime types as well, which simplified the tiny public API some more. |
dhh commentedFeb 10, 2016
Lovely! We have one of those annoying post_json helpers as well. |
…lpersAdd `as` to encode a request as a specific mime type.
-Fixesrails#25183.- The `as: :json` feature was added inrails#21671 and recommended to use for JSON endpoints so let's use it by default for API controller tests.
Turns longish integration test setup like
into
And removes the need to add helpers like
post_jsonetc.I'm not sure if
encoderis the term to use internally. Perhapsinterpreterwould be better. Suggestions welcome.Perhaps we'd need more mime types. Suggestions welcome.