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

Quick fix for os.system requiring str parameter #1005

Merged
merged 3 commits into from
Nov 17, 2011

Conversation

jstenar
Copy link
Member

@jstenar jstenar commented Nov 16, 2011

This is work in progress that needs some feedback

os.system seems to require a str parameter, unicode is converted using ascii.

How should we handle this in a python3 compatible way? Should we extract system_raw to utils/py3compat.py?

@takluyver
Copy link
Member

I expect Python 3 os.system will want unicode. If we know we're always starting with unicode, put it through py3compat.unicode_to_str (which encodes on Python 2 only). If the input can be either unicode or bytes on Python 2, use py3compat.cast_bytes_py2. There's a list of what's in py3compat here: http://wiki.ipython.org/Py3compat_module

@jstenar
Copy link
Member Author

jstenar commented Nov 16, 2011

Thomas skrev 2011-11-16 19:42:

I expect Python 3 os.system will want unicode. If we know we're always starting with unicode, put it through py3compat.unicode_to_str (which encodes on Python 2 only). If the input can be either unicode or bytes, use py3compat.cast_bytes_py2. There's a list of what's in py3compat here: http://wiki.ipython.org/Py3compat_module

I pushed a change using unicode_to_str which seems to do the trick. I've
tried it both on python2 and python3.

@takluyver
Copy link
Member

Looks sensible to me - thanks, Jörgen. I guess there isn't a good way to test this, because it would rely on the existence of a particular network share.

I'll merge this soon, unless anyone objects or beats me to it.

@minrk
Copy link
Member

minrk commented Nov 16, 2011

We should definitely add a unittest that just makes a simple !føø call, to protect from future failures to tackle unicode. The UNC share is a separate issue that is impossible to test, and we just have to be diligent (which I apparently failed to do, because while I did test on UNC shares, I do not have any non-ascii unicode shares).

@minrk
Copy link
Member

minrk commented Nov 16, 2011

Also note that this bug affects all platforms, so both calls to os.system should be protected.

@jstenar
Copy link
Member Author

jstenar commented Nov 17, 2011

I have also added the conversion to the non-windows code-path

@takluyver takluyver merged commit b206d90 into ipython:master Nov 17, 2011
@takluyver
Copy link
Member

Thanks. Merged, with a tiny fix for Python 3.

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.

3 participants