Skip to content

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

Merged
jaraco merged 1 commit into
pypa:masterfrom
benjaoming:patch-1
Jun 3, 2016
Merged

Ensure package name isn't unicode in Python2 distutils#597
jaraco merged 1 commit into
pypa:masterfrom
benjaoming:patch-1

Conversation

@benjaoming

Copy link
Copy Markdown
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

jaraco commented May 28, 2016

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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

jaraco commented Jun 2, 2016

Copy link
Copy Markdown
Member

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
Copy Markdown
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

jaraco commented Jun 3, 2016

Copy link
Copy Markdown
Member

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