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

Convert elements of sys.argv to type newstr to work around a bug in python-future #299

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maxalbert
Copy link
Contributor

This PR is a workaround for a bug in python-future which makes the test isinstance(..., str) fail when applied to strings read from the command line via sys.argv. This bug has caused me problems on Python 2.7 when reading parameters from the command line. The attached PR fixes this.

However, we should only consider this a temporary solution. Once the bug is fixed upstream in python-future we should revert this change.

@timtroendle
Copy link
Contributor

I wouldn't consider this a bug in python-future. Python-future introduces a new string type in Python2 that is supposed to behave similarly to the builtin string type of Python3. That new string type is not supposed to replace -- and can't replace -- the builtin Python2 string type. Without changing the sys module, you won't be able to let sys.argv return the python-future new string type.

So I guess the cleanest solution would be to not perform isinstance tests. As that is not applicable I suppose, there are two other options to take:

  1. wrapping every incoming Py2 builtin-str with newstr, as you are doing here for the command-line arguments (and as I did at a few places)
  2. testing for both string types

Both solutions have the drawback that in case of a complete Py3-shift in the future, they would become obsolete and needed to be removed. So a consistent comment marking these places would be helpful.

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.

2 participants