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 most py2 compat code #2163

Merged
merged 19 commits into from Jul 13, 2019

Conversation

@matthewturk
Copy link
Member

commented Feb 27, 2019

This removes most of the python2 compatibility code. There are a
handful of things that we no longer need to do, specifically lots of
class Something(object): invocations.

Unit tests pass; I anticipate there may be issues in answer testing
related to mistakes in importing or decode/encode of strings, and I look
forward to fixing those.

@ngoldbaum

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

One thing that I'm a little nervous about on the yt-4.0 branch, especially for a big invasive change like this, is that we don't have great test coverage for the frontends because travis and appveyor don't run tests with real data.

It would be great if we could get Jenkins running against the yt-4.0 branch next week.

@matthewturk

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

OK, so this is a non-starter until then?

@ngoldbaum

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Make sure python_requires in setup.py gets updated, it can now just be >=3.5 I think.

@ngoldbaum

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Getting this working would definitely be worthwhile since you'll have to do this annoying mechanical work anyway, there just might be more brokenness in the frontends or in code that only has tests that uses real data that you won't notice until jenkins gets turned on so this probably can't be merged until then.

@matthewturk matthewturk added this to In progress in yt-4.0 Mar 6, 2019

@matthewturk matthewturk added the triage label Mar 6, 2019

@matthewturk matthewturk added this to the 4.0 milestone Mar 7, 2019

@matthewturk

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

To close the loop, this is dependent on #2208 and #2172 , and then (pending those tests passing) I believe it is ready to go.

@matthewturk matthewturk requested review from munkm, Xarthisius and brittonsmith May 8, 2019

@munkm

This comment has been minimized.

Copy link
Member

commented May 16, 2019

I think this is now also dependent on #2260

@brittonsmith
Copy link
Member

left a comment

These all look really straightforward to me. I think this is good.

@jzuhone

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

LGTM!

setup.py Outdated Show resolved Hide resolved
@chummels
Copy link
Member

left a comment

I looked over this, and everything seems to be in order. I think there is just one small thing that could improve this PR. Maybe we could include a check when yt gets imported by including code like this in the __init__.py:

import sys
if sys.version_info[0] < 3:
    raise Exception("Python 2 no longer supported.  Please install Python 3 for use with yt.")
@pep8speaks

This comment has been minimized.

Copy link

commented Jul 12, 2019

Hi there, @matthewturk! Thanks for updating this PR.

Line 21:9: E117 over-indented

@matthewturk matthewturk requested a review from chummels Jul 12, 2019

@matthewturk

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2019

OK! I have merged, and I think this is ready-for-review again.

I have addressed this, and I think Cameron might be away for a while!

@munkm munkm merged commit eaf9dc5 into yt-project:yt-4.0 Jul 13, 2019

5 checks passed

WIP Ready for review
Details
codecov/patch 54.16% of diff hit (target 48.21%)
Details
codecov/project Absolute coverage decreased by -0.06% but relative coverage increased by +5.95% compared to 33dcf44
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

yt-4.0 automation moved this from In progress to Done Jul 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.