Skip to content

Remove Python 2 vestiges #729

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

Closed
markcampanelli opened this issue May 20, 2019 · 3 comments · Fixed by #757
Closed

Remove Python 2 vestiges #729

markcampanelli opened this issue May 20, 2019 · 3 comments · Fixed by #757
Milestone

Comments

@markcampanelli
Copy link
Contributor

With the merging of #728, we can now consider dropping Python2-specific code and workarounds.

Some candidates are:

@wholmgren wholmgren added this to the 0.7.0 milestone Jun 18, 2019
@alexandermorgan
Copy link
Contributor

@markcampanelli your list above is good, but the first point seems to be a separate issue, since it's not clear to me that the current behavior returns the correct number at all, in py2 or py3. At least that's the way the comment on line 214 makes it sound.
I've made a PR that addresses the future imports. They're mostly importing division, but also one print_statement for the versioneer. Should this versioneer future import be left alone since it's a separate entity? has_python2 checks are also removed from conftest.py and test_conftest.py.
Do you agree with the plan of closing this issue after merging my PR #757, and then making a new issue to address your first point above?

@markcampanelli
Copy link
Contributor Author

I think that first item was simply what I discovered from just searching the code for Python 2 references. I think it's fine to address that in a separate issue/PR. I don't see a problem with also removing the versioneer future presently if it is not needed.

I think you should also search the code for "python 2" and variants. For example, this identifies import checks that are no longer needed, such as

from urllib2 import urlopen, Request

@alexandermorgan
Copy link
Contributor

Thanks, I've sorted out all the py2-compatible imports. Also, after reading the commit where the comment about python 2 was added in atmosphere.py, I see that we don't need to make a new issue for that. We can just remove the comment because the solution previously found seems to work fine in py2 and py3, and looks a little better than the approach that was only py3 compatible, so no problems there after all.

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

Successfully merging a pull request may close this issue.

3 participants