Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Raise ValueError on Response.encoding being set after Response.text has been accessed #2852

Merged
merged 3 commits into from
Sep 19, 2023
Merged

Raise ValueError on Response.encoding being set after Response.text has been accessed #2852

merged 3 commits into from
Sep 19, 2023

Conversation

xzmeng
Copy link
Contributor

@xzmeng xzmeng commented Sep 17, 2023

Discussed in #2041

Summary

After Response.text is accessed, if you try to change Response.encoding, nothing happens. So just raise an Exception on change to encoding after text is accessed.

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.

@tomchristie
Copy link
Member

Okay yes, as discussed this is probably a marginal improvement in behaviour. Thanks.

tomchristie
tomchristie requested changes Sep 17, 2023
View reviewed changes
tests/models/test_responses.py Outdated Show resolved Hide resolved
@xzmeng xzmeng requested a review from tomchristie September 17, 2023 21:27
@tomchristie
Copy link
Member

What's a more precise title for this PR?

@xzmeng xzmeng changed the title Raise ValueError on change encoding Raise ValueError on Response.encoding being set after Response.text has been accessed Sep 18, 2023
@xzmeng
Copy link
Contributor Author

xzmeng commented Sep 18, 2023

Modified. I'm not proficient in English, please change it if it's not appropriate.

tomchristie
tomchristie reviewed Sep 18, 2023
View reviewed changes
Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

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

All looking great, thanks. 😊

I think we just need to update the CHANGELOG.md, then this is good to go.

@tomchristie tomchristie merged commit 59df819 into encode:master Sep 19, 2023
@tomchristie tomchristie mentioned this pull request Nov 2, 2023
Merged
@zevisert zevisert mentioned this pull request Jan 4, 2024
Closed
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@karpetrosyan karpetrosyan karpetrosyan approved these changes

@tomchristie tomchristie tomchristie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xzmeng @tomchristie @karpetrosyan