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

Remove compatibility shim for bytes type #1479

Merged
merged 2 commits into from
Sep 16, 2018
Merged

Remove compatibility shim for bytes type #1479

merged 2 commits into from
Sep 16, 2018

Conversation

jdufresne
Copy link
Contributor

@jdufresne jdufresne commented Sep 14, 2018

Summary of changes

The type bytes is available on all supported Pythons. Makes the code more forward compatible with Python 3.

Pull Request Checklist

  • News fragment added in changelog.d. See documentation for details

Copy link
Member

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

This looks beautiful, but could we separate the two concerns (replacing binary_type with bytes from correcting encode to decode)?

if isinstance(pattern, binary_type):
dirname = os.curdir.encode('ASCII')
if isinstance(pattern, bytes):
dirname = os.curdir.decode('ASCII')
Copy link
Member

Choose a reason for hiding this comment

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

So while I think you're right about this change (encode->decode), it should probably be in a separate commit and maybe even a separate PR, as it's very different from the stated intention of the commit.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is wrong: the goal of glob is to support this usage too:

python -c 'from setuptools.glob import glob; print(glob(b"*.rst"))'

no?
in any case, this fails with this PR.

Copy link
Member

Choose a reason for hiding this comment

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

On master, from setuptools directory, the results are:

[b'README.rst', b'towncrier_template.rst', b'CHANGES.rst']

Copy link
Member

Choose a reason for hiding this comment

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

This is probably a good time to add some tests.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like the intent of the patch was to change to:

dirname = os.curdir.encode('ASCII')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! You're right. I had that wrong. It should be dirname = os.curdir.encode('ASCII'). I've corrected this in the latest revision.

if isinstance(dirname, binary_type):
dirname = binary_type(os.curdir, 'ASCII')
if isinstance(dirname, bytes):
dirname = os.curdir.decode('ASCII')
Copy link
Member

Choose a reason for hiding this comment

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

Again, this seems like a change to do more than just replace binary_type with bytes. How did this code work before?

@jdufresne
Copy link
Contributor Author

This is probably a good time to add some tests.

There is no behavioral change. It is a pure internal refactoring. Do you still want to see tests for this PR? Let me know. If so, I'll add them.

@benoit-pierre
Copy link
Member

Do you still want to see tests for this PR? Let me know. If so, I'll add them.

Yes, here are some simple ones to prevent future regressions:

import pytest

from setuptools.glob import glob

from .files import build_files


@pytest.mark.parametrize('tree, pattern, matches', (
    ('', b'', []),
    ('', '', []),
    ('''
     appveyor.yml
     CHANGES.rst
     LICENSE
     MANIFEST.in
     pyproject.toml
     README.rst
     setup.cfg
     setup.py
     ''', '*.rst', ('CHANGES.rst', 'README.rst')),
    ('''
     appveyor.yml
     CHANGES.rst
     LICENSE
     MANIFEST.in
     pyproject.toml
     README.rst
     setup.cfg
     setup.py
     ''', b'*.rst', (b'CHANGES.rst', b'README.rst')),
))
def test_glob(monkeypatch, tmpdir, tree, pattern, matches):
    monkeypatch.chdir(tmpdir)
    build_files({name: '' for name in tree.split()})
    assert list(sorted(glob(pattern))) == list(sorted(matches))

To be put in setuptools/tests/test_glob.py.

The type bytes is available on all supported Pythons. Makes the code
more forward compatible with Python 3.
@jdufresne
Copy link
Contributor Author

Thanks. I've added the suggested tests (which do fail with the mistake I temporarily had).

@jaraco jaraco merged commit 40aabe3 into pypa:master Sep 16, 2018
@jdufresne jdufresne deleted the bytes branch October 19, 2019 15:48
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.

None yet

3 participants