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

Use stream encoding instead of locale preferred encoding #88

Merged
merged 2 commits into from
Jan 5, 2023

Conversation

Delgan
Copy link
Collaborator

@Delgan Delgan commented May 25, 2019

Hi.

This is intended to fix #53, finally. :)

As discussed, I replaced locale.getpreferredencoding() with STREAM.encoding. Just in case, I added a fallback to ascii if STREAM has no or invalid encoding attribute. For the tests, the encoding can be configured with the PYTHONIOENCODING environment variable in place of LANG and LC_ALL. Also, I had to remove encoding.py and include it directly into color.py to avoid circular dependency.

Hopefully, this should solve reported encoding issues. ;)



STREAM = sys.stderr
ENCODING = getattr(STREAM, "encoding", None) or "ascii"
Copy link
Owner

Choose a reason for hiding this comment

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

Better to default to utf-8 IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Qix- Yeah, I think you are right, that would avoid possible UnicodeEncodeError.

@Qix-
Copy link
Owner

Qix- commented Sep 28, 2019

@Delgan does this supercede #69?

@Delgan
Copy link
Collaborator Author

Delgan commented Sep 28, 2019

@Delgan does this supercede #69?

@Qix- There is definitely overlap between #88 and #69, but #69 tried to go further by removing a bunch of hacky functions used for Python 2 compatibility with colorama.
Re-reading what I commented on the PR at that time, it seems this was mostly personal convictions about how encoding should be done. This did not solve any actual issue, so it's ok to merge #88 and reject #69 (I could simply re-implement the changes in the future if needed).

@Qix-
Copy link
Owner

Qix- commented Sep 28, 2019

Sounds good :)

@danyeaw
Copy link

danyeaw commented Dec 29, 2022

PEP597 Encoding Warnings gives warnings about locale preferred encoding if you have the warnings enabled. What needs to happen to get this PR merged?

@Qix-
Copy link
Owner

Qix- commented Jan 2, 2023

Just a rebase on top of current master. I'm out of the country at the moment so my capacity for code changes is low. If @Delgan does a rebase or if you submit a new PR that is rebased I'll accept it, though it'll be a little bit before I can do a release.

@Delgan
Copy link
Collaborator Author

Delgan commented Jan 3, 2023

Hey there. :)

I just rebased the PR. 👍
It's quite old, but I first glance, it seems the proposed workaround is still relevant.

@Qix- Qix- merged commit f7f1476 into Qix-:master Jan 5, 2023
@Qix-
Copy link
Owner

Qix- commented Jan 5, 2023

It's going to be a little bit before I can push a release, sorry in advance. @Delgan if you're interested in being added as a pypi maintainer just let me know what your pypi username is.

@Delgan
Copy link
Collaborator Author

Delgan commented Jan 6, 2023

@Qix- Thank you for your trust, but I honestly prefer not to handle the administrative part. Let me know if you need help with other developments, though. I would be happy to contribute. 👍

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.

Display coding problem
3 participants