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

Remove sys.setdefaultencoding #1963

Closed
Tracked by #10484
rtibbles opened this issue Aug 7, 2017 · 6 comments
Closed
Tracked by #10484

Remove sys.setdefaultencoding #1963

rtibbles opened this issue Aug 7, 2017 · 6 comments
Assignees
Labels
TAG: tech update / debt Change not visible to user

Comments

@rtibbles
Copy link
Member

rtibbles commented Aug 7, 2017

Summary

(filled in by @benjaoming)

As I comment on ba2a383

Sorry, but we can't allow code to be injected in strange places like this. Everything that has to do with Python 2 compatibility has to be put in the right place and properly explained.

Please see various dangers and reasons for not using it...

https://stackoverflow.com/a/3828742/405682

@benjaoming
Copy link
Contributor

@rtibbles just curious, how did you recall this issue?

Also, @ralphiee22 can you clarify why this was needed since your commit comment is "Fly by fixes for kolibri and tasks." ;)

@rtibbles
Copy link
Member Author

@benjaoming it was on the Kolibri Tasks trello board, on the Tech Debt list. I migrated all the cards from there to github issues.

@ralphiee22
Copy link
Contributor

@benjaoming unicode decode/encode hell? Can't recall if because strings were not coming in as unicode or python was not reading them as unicode.

@benjaoming
Copy link
Contributor

Since this is so clearly discouraged, I'll try removing it and see what happens. Then we can add encoding and decoding fixes where they are needed - to have an explicit handling of wrong encodings and possibly track them down to their source.

IMO the only way to deal with encoding issues is at the source, the possible issues that are mentioned in the StackOverflow answer do not sound nice to try and track down... especially in some distant future where maintaining Python 2.7 support is a terrible burden to someone.

I suggest:

  1. We isolate all Python 2.7 compatibility handling to a central location (I've suggested this before) - in this example, there's some compat code sitting in the middle of module load time.

  2. We try dealing with terminal encoding by having utility functions for all terminal invocation - this will also make it easier to mock stuff for testing purposes

  3. All encoding issues from data (CC server etc) should be dealt with at the source. Any problems that occur in this regard are IMO fine, we just need to fix them, when we see them. Since Python 3 has UTF-8 encoding and decoding built into a lot of functions, we won't see these issues in anything but Python 2.7 -- however, if we do see them, we would know that some data we are using was wrongly encoded and has a danger of getting wrongly interpreted at some point.

@indirectlylit indirectlylit added TODO: needs gherkin update Add to our manual integration tests TAG: tech update / debt Change not visible to user labels Oct 27, 2017
@indirectlylit indirectlylit added this to the 0.8.0 milestone Dec 15, 2017
@indirectlylit indirectlylit added P1 - important Priority: High impact on UX P2 - normal Priority: Nice to have and removed P1 - important Priority: High impact on UX labels Dec 15, 2017
@indirectlylit indirectlylit removed this from the 0.8.0 milestone Feb 26, 2018
@indirectlylit indirectlylit removed the P2 - normal Priority: Nice to have label Feb 26, 2018
@indirectlylit indirectlylit removed the TODO: needs gherkin update Add to our manual integration tests label Mar 12, 2019
@rtibbles
Copy link
Member Author

Note that the location of this code has now migrated to: https://github.com/learningequality/kolibri/blob/release-v0.14.x/kolibri/utils/env.py#L63

I think the simplest solution to this issue, may be just to handle it when we eventually drop Python 2 support.

@rtibbles
Copy link
Member Author

rtibbles commented Jan 2, 2024

Fixed in #11654

@rtibbles rtibbles closed this as completed Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TAG: tech update / debt Change not visible to user
Projects
None yet
Development

No branches or pull requests

4 participants