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

Fixed helpers.py for Python 3.7 (PEP 479) #462

Merged
merged 4 commits into from Nov 30, 2018

Conversation

dhait
Copy link
Contributor

@dhait dhait commented Nov 18, 2018

Modification to fix nosetest helpers.py failing due to changes in Python 3.7 (PEP 479)

nosetests -v and tox -e py27,py36,py37 all passed after modifications.

No new unit tests
No new functions

@coveralls
Copy link

coveralls commented Nov 18, 2018

Coverage Status

Coverage increased (+0.2%) to 90.291% when pulling ee0a196 on OptionMetrics:master into 9372b2a on petl-developers:master.

@dhait
Copy link
Contributor Author

dhait commented Nov 18, 2018

Ouch. 2 tiny lines drops the coverage to 89.987. Any quick suggestions?

Copy link
Collaborator

@alimanfoo alimanfoo left a comment

Choose a reason for hiding this comment

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

Thanks @dhait, much appreciated. If we're adding support for PY37 then I think it's worth dealing with PEP 479 compatibility fully, see comments. Also PY37 should be added to appveyor.yml so we test under windows.

.travis.yml Outdated
@@ -3,6 +3,7 @@ branches:
- master
language: python
python:
- "3.7-dev"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Which version of Python do you get with this? FWIW I think it should be possible to use latest release 3.7, I did recently for another project, see this .travis.yml.

a = cast(a)
eq_(e, a)
except:
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is needed because there is code inside petl that raises StopIteration from inside a generator, then I think it would be better to fix the code rather the test. To deal with PEP 479 change I believe we can just replace "raise StopIteration" with a bare "return" instead.

Copy link
Collaborator

@alimanfoo alimanfoo left a comment

Choose a reason for hiding this comment

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

Thanks @dhait, LGTM. I'll wait to check CI passes.

@alimanfoo alimanfoo added this to the v1.3 milestone Nov 19, 2018
@dhait dhait mentioned this pull request Nov 30, 2018
@alimanfoo alimanfoo merged commit 2cb5dc5 into petl-developers:master Nov 30, 2018
@alimanfoo
Copy link
Collaborator

Thanks again @dhait.

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