Skip to content

pass URL-encoded form data#13

Merged
foxzool merged 2 commits into
foxzool:masterfrom
izarma:master
May 29, 2026
Merged

pass URL-encoded form data#13
foxzool merged 2 commits into
foxzool:masterfrom
izarma:master

Conversation

@izarma
Copy link
Copy Markdown
Contributor

@izarma izarma commented Nov 2, 2025

added a function form_encoded to pass URL-encoded form data in the request body.
wanted to discuss if the headers i'm adding by default are fine?

Copy link
Copy Markdown
Owner

@foxzool foxzool left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Accept header from form_encoded()
  • Keep the whitespace change out of the diff

Happy to re-review once it's on a feature branch. Thanks again!

@foxzool foxzool merged commit dd8774f into foxzool:master May 29, 2026
5 checks passed
@izarma
Copy link
Copy Markdown
Contributor Author

izarma commented May 29, 2026

Thank you for the review and sorry you had to merge this as is.

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.

2 participants