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

Drop support for Python 2 #2016

Merged
merged 6 commits into from Jan 31, 2019

Conversation

Projects
None yet
3 participants
@hugovk
Copy link
Contributor

hugovk commented Jan 30, 2019

Some more stuff for #1963.

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Jan 30, 2019

@hugovk - Thanks!

@bsipocz - is ah_bootstrap.py a file we should update?

It's imported in setup.py, and using Python 3 stuff in setup.py might not be a good idea (according to https://python3statement.org/practicalities/)

@hugovk - did you run an updater on ah_bootstrap.py or is this an updated copy from https://github.com/astropy/package-template/blob/master/ah_bootstrap.py ?

I'm off now, will look at this tomorrow morning.

@hugovk

This comment has been minimized.

Copy link
Contributor Author

hugovk commented Jan 30, 2019

@cdeil You're welcome! I ran an updater and made manual changes on ah_bootstrap.py. If it's an upstream file, it's probably better to leave it be. Shall I revert the changes in that file?

@bsipocz

This comment has been minimized.

Copy link
Member

bsipocz commented Jan 30, 2019

Leave ah_bootstrap.py out of manual updates (I'm fairly certain it doesn't like to being updated while the helpers submodule is not yet updated), even the old one works perfectly with python3. Otherwise the update is done in #2017

@hugovk

This comment has been minimized.

Copy link
Contributor Author

hugovk commented Jan 30, 2019

Leave ah_bootstrap.py out of manual updates, even the old one works perfectly with python3. Otherwise the update is done in #2017

Done!

@cdeil cdeil self-assigned this Jan 31, 2019

@cdeil cdeil added the cleanup label Jan 31, 2019

@cdeil cdeil added this to the 0.11 milestone Jan 31, 2019

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Jan 31, 2019

@hugovk - Thank you very much!

I think I had seen https://github.com/asottile/pyupgrade in the past, but didn't remember or find it with Google yesterday. I wish I had, it's exactly the tool that would have done the transformations I applied manually automatically. Overall the Gammapy codebase isn't very large, so with multi-file search and replace it took me less than an hour overall, but still.

Please make some changes:

  • gammapy/extern/appdirs.py is an extern / bundled file, we don't want to edit / maintain it. Please revert any edits there.
  • The edits for setup.py and I think also for _astropy_init.py should be reverted. The recommended way is to leave them such that a clear error message is given if someone tries to use it with Python 2. Currently we have this:
$ python2 setup.py install
ERROR: Python (3, 5) or later is required by astropy-helpers

With the edits you make here, it fails with an ImportError that Python newbies won't understand:

$ python2 setup.py install
Traceback (most recent call last):
  File "setup.py", line 9, in <module>
    import builtins
ImportError: No module named builtins
  • Would you be willing to squash the commits here? To avoid having edits / back and forth in critical files like setup.py in our project version history?
@hugovk

This comment has been minimized.

Copy link
Contributor Author

hugovk commented Jan 31, 2019

I've reverted those files. Sure, happy to squash commits.

Let me know when the PR is ready and I'll do it, or you can do it during merge with the "Squash and merge" button:

https://help.github.com/articles/merging-a-pull-request/#merging-a-pull-request-on-github

@cdeil

cdeil approved these changes Jan 31, 2019

@cdeil cdeil merged commit fdf4870 into gammapy:master Jan 31, 2019

2 of 3 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
Scrutinizer Analysis: No new issues – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Jan 31, 2019

"Squash and merge" button

It's the first time I used this. Seems to work great. Looks like this, one commit in master, no separate merge commit: fdf4870

So far I never squashed for contributions, because I wanted to have credit preserved. But I guess "hugovk authored and cdeil committed" is OK, i.e. authorship credit is preserved well-enough.

@hugovk - Thank you very much!

@cdeil cdeil referenced this pull request Jan 31, 2019

Closed

Remove Python 2 support #1963

@hugovk

This comment has been minimized.

Copy link
Contributor Author

hugovk commented Jan 31, 2019

All good, thanks!

@hugovk hugovk deleted the hugovk:rm-2 branch Jan 31, 2019

@bsipocz

This comment has been minimized.

Copy link
Member

bsipocz commented Feb 1, 2019

So far I never squashed for contributions, because I wanted to have credit preserved. But I guess "hugovk authored and cdeil committed" is OK, i.e. authorship credit is preserved well-enough.

The credit will still go to the author in this case, I haven't seen it anywhere where the committer gets listed for a commit and not the author.

@bsipocz

This comment has been minimized.

Copy link
Member

bsipocz commented Feb 1, 2019

$ python2 setup.py install
ERROR: Python (3, 5) or later is required by astropy-helpers

This will be a cleaner error message with the 3.2 version of the helpers, saying that gammapy requires python 3.5 or later.

@bsipocz

This comment has been minimized.

Copy link
Member

bsipocz commented Feb 1, 2019

(I'm afraid we didn't backported those changes to the 3.1.x branch, so the release will be out with astropy 3.2 in a few months along with tons of other simplifications in the helpers).

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.