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

feat(content): allow multipart requests without files. #3397

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

laipz8200
Copy link

@laipz8200 laipz8200 commented Nov 8, 2024

Summary

related #3396.

Currently, a multipart request can only be sent when the files parameter is non-empty. This restriction limits cases where users might want to send data using multipart mode without attaching any files, which is possible in tools like Postman.

Change: Instead of requiring files to be non-empty, we could determine the need for a multipart request based on whether files is None rather than its emptiness. This would allow users to send multipart requests without attaching files if needed.

I think there is no significant documentation update required, so I have not made any changes. However, please let me know if any updates are needed.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@laipz8200 laipz8200 marked this pull request as ready for review November 8, 2024 07:02
tests/test_content.py Outdated Show resolved Hide resolved
tests/test_content.py Outdated Show resolved Hide resolved
tests/test_content.py Outdated Show resolved Hide resolved
@tomchristie
Copy link
Member

Thanks. I've updated the tests to avoid private imports.

Do we think this is an improved behaviour? It does seem nice that an explicit files={} will enforce multipart. (On the other hand, if data and files are determined dynamically as in the cli we need to explicitly switch to None)

tests/test_content.py Outdated Show resolved Hide resolved
@laipz8200
Copy link
Author

Thanks for the update!

My original plan was to make it easier to use multipart without having to carry files, which wasn’t as straightforward before.

I don’t use the cli much, so if this causes any issues, just let me know.

@@ -483,7 +483,7 @@ def main(
params=list(params),
content=content,
data=dict(data),
files=files, # type: ignore
files=files or None, # type: ignore
Copy link

Choose a reason for hiding this comment

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

I don't think this change is correct. It seems to go against the spirit of your main change in the httpx/_content.py file.
It seems to prevent the fix from working when using the cli.
Although I am not familiar with this usage and I didn't look too much into it, so I don't know if it's even possible to do this through the cli.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the review, I'll take a look at the CLI.

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