search

astropy / astroplan

Observation planning package for astronomers – maintainer @bmorris3
https://astroplan.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
198 stars 109 forks source link

Remove deprecated call to astropy helpers. #554

Open wtgee opened 1 year ago

wtgee commented 1 year ago

Removes the function call and defaults to pytest just showing the warning at end of test run. Thanks @pllim for pointing out.

Edit: Astropy issue here https://github.com/astropy/astropy/issues/9718 and PR https://github.com/astropy/astropy/pull/14670

Closes #553

P.S. I feel like there is some kind of meta irony in us not noticing this was deprecated...

pllim commented 1 year ago

To retain the functionality you are removing here, you need to change

https://github.com/astropy/astroplan/blob/cb9517e381a3e8338e8d182e5f130201139f7fc7/setup.cfg#L44

to turn warning into exceptions using

https://docs.pytest.org/en/stable/how-to/capture-warnings.html

wtgee commented 1 year ago

Now I'm confused why this wasn't working before. Currently passing PRs show deprecations as warnings rather than raising actual exceptions.

pllim commented 1 year ago

The pytest way is probably more sensitive than what astropy was providing.

pllim commented 1 year ago

You should probably drop Python 3.7. It is picking up a very old astropy (4.3.1).

wtgee commented 1 year ago

You should probably drop Python 3.7. It is picking up a very old astropy (4.3.1).

@bmorris3 I was going to make the same recommendation. End of life for 3.7 is at the end of this month so seems like a good time to remove.

Edit: The test matrix in general could probably use some updating as it specifies older astropy versions specifically in addition to python 3.7. The tox.ini and ci_tests.yml also don't match up, although that's probably okay (?).

bmorris3 commented 1 year ago

Hi @wtgee, sorry to begin reviewing this PR after making and merging #558, which I believe has the same effect as this PR. Shall we close this one?

wtgee commented 1 year ago

Hey @bmorris3, this PR properly turns on the deprecations as exceptions via pytest, which replaces the functionality that was supposed to be working with enable_deprecations_as_exception. #558 removes that but doesn't add the pytest filter.

It's up to you if that should be in the repo but adding it in shows a bunch of other deprecations that should probably be dealt with soon (hence the follow-up issues about python 3.7, etc), so it's arguably nice to retain that functionality.