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.

Sign up for GitHub

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

Jump to bottom

fixed #1042 -- corrected an import #1043

Merged
merged 1 commit into from
Jun 1, 2017
Merged

fixed #1042 -- corrected an import #1043

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion setuptools/py27compat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import platform

import six
from setuptools.extern import six
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why using six???

In package six the Python version is determined by calling sys.

PY2 = sys.version_info[0] == 2
See: https://bitbucket.org/gutworth/six/src/92e1c746e1abfbdb94705b821bc93766083efc54/six.py?at=default&fileviewer=file-view-default

Best solution is to use "sys" to check if it is python 2 version.

If it must be version 2.7 check for major and minor version!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert 1d928cb ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually reverting should looks better.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, reverting is a much cleaner solution.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all people still wait, if you can please revert this shortly

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

six is already used elsewhere, so it makes sense to use it here as well (after the import is corrected).

@westsouthnight This was worked around by pulling the affected version from PyPI, so things should be fine again for now.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have seen that it was used in other parts to check if an object is of the type "isinstance(value, six.string_types)". Does that mean that this part of the code must have six? I just can't see that "import sys" must be replaced by six. It doesn't solve a problem in my opinion.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fwdevmobile It doesn't mean it must have six. But it makes sense for it to use it when available, to - even if only for a tiny bit - increase readability and reduce technical debt. It's unfortunate that there was a mistake in it, but generally, it - IMHO - is a slight improvement.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree with the new import will solve all deployment issues. Please merge.

from setuptools.extern import six



def get_all_headers(message, key):
Expand Down