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: Make encoding_rs an optional feature #2175

Merged
merged 2 commits into from
Mar 19, 2024

Conversation

Marwes
Copy link
Contributor

@Marwes Marwes commented Mar 13, 2024

Keeping it as a default feature so that the default user experience isn't to be confused about the lack of text. It is possible that text (but not text_with_charset) exists without the encoding_rs feature by only trying to decode the response as utf-8 in that case. However that would mean that the behaviour changes if the feature is later added since the method would start to respect Content-Type then.

Closes #1785

@seanmonstar seanmonstar changed the base branch from master to 0.12-dev March 13, 2024 13:02
@seanmonstar
Copy link
Owner

If you base it off the 0.12-dev branch, it should clear up the conflicts. (Or it might need a rebase or a new branch and cherry-pick the commit, unsure). That's where I'm staging all the breaking changes.

@Marwes Marwes force-pushed the encoding_feature branch 2 times, most recently from 982d664 to b6ffdc9 Compare March 13, 2024 15:55
@Marwes
Copy link
Contributor Author

Marwes commented Mar 13, 2024

Getting some test failures because text is now missing with --no-default-features, unsure how you want this to be fixed, changing the tests to use bytes(), disable them for this case or to try and enable a limited form of text() without encoding_rs.

Feel free to close this PR and take or remake the changes if that is easier.

@seanmonstar
Copy link
Owner

I think I'm leaning towards text just using from_utf8 without default features, and documentation that says it can decode more if you enable the charset feature. What do you think?

@Marwes Marwes force-pushed the encoding_feature branch 2 times, most recently from a81edae to aa59fc5 Compare March 19, 2024 17:06
@Marwes Marwes force-pushed the encoding_feature branch from 46f103d to 17d6d5f Compare March 19, 2024 17:11
Copy link
Owner

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Wonderful! Thanks!

@seanmonstar seanmonstar merged commit 8735cb8 into seanmonstar:0.12-dev Mar 19, 2024
32 checks passed
@Marwes Marwes deleted the encoding_feature branch March 20, 2024 09:24
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.

Put encoding_rs dependency behind a Cargo feature?
2 participants