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

Ensure package name isn't unicode in Python2 distutils #597

Merged
merged 1 commit into from
Jun 3, 2016

Conversation

benjaoming
Copy link
Contributor

Problem encountered on Ubuntu 14.04

Fixes #190

If people need to fix this without this patch, they have to wrap their package names in str() like so:

#!/usr/bin/env python
# -*- coding: utf-8 -*-
from __future__ import absolute_import, print_function, unicode_literals

setup(
    # ....
    package_dir=[
        str('package_name'),
    ]
)

benjaoming added a commit to benjaoming/kolibri that referenced this pull request May 27, 2016
@jaraco
Copy link
Member

jaraco commented May 28, 2016

Since the issue only affects Python 2, I'd expect the fix to only affect Python 2, which would also provide a clearer indication of the scope of the error. As written, I wouldn't expect my future self to know that this code can be removed when Python 2.7 support is removed. Also, why select on string_types? Can package_name ever be anything other than a string type?

@benjaoming
Copy link
Contributor Author

@jaraco -- you are right, and I didn't research if later Python 2 distutils versions have fixed this. I think it would be okay to not care which distutils versions exactly are causing the issue and simplify this as a Python 2 fix, expressing that in the comments...?

@benjaoming
Copy link
Contributor Author

Also, why select on string_types? Can package_name ever be anything other than a string type?

Yes. Read #190:

TypeError: 'package' must be a string (dot-separated), list, or tuple

@jaraco
Copy link
Member

jaraco commented Jun 2, 2016

I guess what I'm getting at is that since the Python 3 implementation does the right thing currently, this code shouldn't affect that, but this code does. In particular, the proposed implementation will convert bytes into the repr of their value, masking the underlying error.

How about instead, something like:

if six.PY2 and isinstance(package, six.text_type):
  # avoid errors on Python 2 when unicode is passed (#190)
  package = package.split('.')

That implementation (a) captures the PY2 specific behavior, (b) only affects if unicode is passed, and (c) provides forward-compatibility, allowing for unicode values to be passed and treated just like unicode values are handled on Python 3.

@benjaoming
Copy link
Contributor Author

Hi @jaraco

In particular, the proposed implementation will convert bytes into the repr of their value, masking the underlying error.

Thanks for dedicating interest and putting thought into a better solution! I agree, it's a better fix, cleaner and doesn't affect Python 3 behaviour.

Squashed into 1 commit and -f'ed.

@jaraco jaraco merged commit 58b12d1 into pypa:master Jun 3, 2016
@jaraco
Copy link
Member

jaraco commented Jun 3, 2016

Thanks for bringing the suggestions to the table and working through my concerns.

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.

Unfriendly error message when unicode is passed to package_dir or packages
2 participants