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

Python 3 fixes - fix tarutil and contextutil_test #6243

Merged
merged 3 commits into from Jul 26, 2018

Conversation

Projects
None yet
2 participants
@Eric-Arellano
Copy link
Contributor

Eric-Arellano commented Jul 26, 2018

Spoke too soon when saying util was all ported.

Tarutil was failing due to a quirk in Py3's implementation of __next__. Usually, iterators in Py3 implement __next__ and in Py2 implement next(). For some reason, Tarutil does not have the dunder __next__ and uses the legacy next(). So, we were failing to use our override of this function.

Now util passes 100% on Python 2 and 3!



@implements_iterator
class TarFile(tarfile.TarFile):

def __next__(self):

This comment has been minimized.

@jsirois

jsirois Jul 26, 2018

Member

This rename in a81f5cd is the real problem and it's misleading, I'd suggest just reverting the rename instead of aliasing here. The tarfile.TarFile code does not implement __next__ in python2 or python3, instead it implents __iter__ with custom returned iterators that are not tarfile.TarFile objects.

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 26, 2018

Contributor

True.

Even though that goes against the general Python 2-3 compatibility advice to have next() for Py2 and __next__() for Py3, you raise a good point we should mirror CPython and not implement __next__() since they don't.

Thanks for the review!

@jsirois jsirois merged commit f948a19 into pantsbuild:master Jul 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:py3-fixes_remaining-util branch Jul 30, 2018

CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment