search

astropy / package-template

Template for packages that use Astropy. Maintainer: @astrofrog
http://docs.astropy.org/projects/package-template/en/latest/
Other
60 stars 63 forks source link

Remove redundant Python version check #388

Closed astrofrog closed 5 years ago

astrofrog commented 5 years ago

Last year we added a Python version check at the top of setup.py to address https://github.com/astropy/astropy/issues/7306. However, since then this has also been added to the top of ah_bootstrap.py. Since ah_bootstrap is imported immediately after the check, we can therefore remove the check in setup.py.

(this is part of a series of PRs to simplify setup.py)

Companion to https://github.com/astropy/astropy/pull/8344

Cadair commented 5 years ago

hmmmm, the advantage of doing it here is it get's the package name from the cookiecutter rather than whatever ah_bootstrap says?

astrofrog commented 5 years ago

Either you are using pip, in which case you will never run into this issue anyway since pip won't even download packages where the python_requires is not compatible, or you are installing the package locally, in which case you know what the package is. So I don't really see the value in having the package name in the error? (also we can easily get that from setup.cfg in ah_bootstrap.py if you really want).

drdavella commented 5 years ago

@astrofrog that does make sense. However, just to be difficult, the case where this would actually appear is when someone is running an older version in Python, in which case it's likely that they have an older version of pip that doesn't actually check for python_requires (which is only supported fairly recently, iirc).

But either way I'm not going to hold it up since it makes good sense for long-term maintenance.

astrofrog commented 5 years ago

@drdavella - won't pip say which package failed to install though? In any case I still prefer the option of including the package name in the ah_bootstrap.py error if we really want.

Cadair commented 5 years ago

@astrofrog I am on board if ah_bootstrap reads setup.cfg, I just don't want the sunpy setup.py thing to say "astropy"...

drdavella commented 5 years ago

Currently it would say "astropy-helpers". So I agree with reading from setup.cfg.

astrofrog commented 5 years ago

Ok I'll work on improving the error message in ah_bootstrap.py

astrofrog commented 5 years ago

@Cadair @drdavella - what about https://github.com/astropy/astropy-helpers/pull/441

astrofrog commented 5 years ago

I think we might have to include this only once we drop Python 2 (which could be shortly, see #385)

bsipocz commented 5 years ago

385 is merged, so I go ahead with this, too. (I've reread the discussion and given the concerns are addressed in the helpers (and has been included on this branch) I go ahead with this.)