-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Make sure we pass unicode, not bytes to CreateProcess() #830
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
Conversation
This eliminates exceptions on Python 3 when passing a path argument encoded as bytes.
…spawnProcess on Windows.
opalmer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
On Windows file system paths are Unicode by default so it's a bit redundant to do .decode(sys.getfilesystemencoding()). The docs for Python 2 mention that getfilesystemencoding() is mainly intended for string -> byte conversion and for Python 3 it apparently changes to always be 'utf-8'. But based on the docs it shouldn't hurt either so I'd leave it as you have it in case there are edge cases out there.
References:
https://docs.python.org/2/library/sys.html#sys.getfilesystemencoding
https://docs.python.org/3/library/sys.html#sys.getfilesystemencoding
https://www.python.org/dev/peps/pep-0529/
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247.aspx
|
Thank you @altendky ! |
opalmer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, forgot to hit approve on my side.
|
What happened to break it again :-( it looks like 95% now. |
|
If I try to look at https://codecov.io/gh/twisted/twisted/pull/830 , I get a 500 error |
|
I see 100% patch coverage here: https://codecov.io/gh/twisted/twisted/pull/830 |
https://twistedmatrix.com/trac/ticket/9200