pass URL-encoded form data#13
Conversation
foxzool
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Adding form_encoded support makes sense for the crate.
However, there are a few issues that need to be addressed before this can be merged:
1. Branch issue
This PR is opened from your master branch, which prevents CI from running and makes it hard to review incrementally. Please open a new PR from a feature branch (e.g. feat/form-encoded).
2. Default Accept: application/json header
You already flagged this yourself in the PR description — and I agree it shouldn't be the default here. URL-encoded form POSTs (OAuth token endpoints, HTML form submissions, etc.) commonly expect text/html, application/x-www-form-urlencoded, or nothing at all in the Accept header. Hardcoding application/json assumes too much about the server's response format. I'd suggest dropping the Accept header entirely from form_encoded() and letting the caller add it via .headers() if they need it.
3. Trailing whitespace cleanup
The unrelated trailing whitespace change on the EmptyArray doc comment line should be a separate PR (or we can just batch-fix formatting separately). Keeping unrelated changes out of feature PRs makes history cleaner.
Next steps:
- Close this PR
- Open a new one from
feat/form-encoded - Drop the
Acceptheader fromform_encoded() - Keep the whitespace change out of the diff
Happy to re-review once it's on a feature branch. Thanks again!
|
Thank you for the review and sorry you had to merge this as is. |
added a function
form_encodedto pass URL-encoded form data in the request body.wanted to discuss if the headers i'm adding by default are fine?