Remove compatibility shim for bytes type#1479
Conversation
jaraco
left a comment
There was a problem hiding this comment.
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') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
On master, from setuptools directory, the results are:
[b'README.rst', b'towncrier_template.rst', b'CHANGES.rst']There was a problem hiding this comment.
This is probably a good time to add some tests.
There was a problem hiding this comment.
It looks like the intent of the patch was to change to:
dirname = os.curdir.encode('ASCII')There was a problem hiding this comment.
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') |
There was a problem hiding this comment.
Again, this seems like a change to do more than just replace binary_type with bytes. How did this code work before?
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. |
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 |
The type bytes is available on all supported Pythons. Makes the code more forward compatible with Python 3.
|
Thanks. I've added the suggested tests (which do fail with the mistake I temporarily had). |
Summary of changes
The type
bytesis available on all supported Pythons. Makes the code more forward compatible with Python 3.Pull Request Checklist