renamed and deprecated Observations and Observations class for the gemini and mast module

[tinuademargaret]

trafficstars

[codecov[bot]]

Codecov Report

Merging #1885 (e68e664) into main (96f63eb) will increase coverage by 1.56%. The diff coverage is 100.00%.

:exclamation: Current head e68e664 differs from pull request most recent head 65cbaad. Consider uploading reports for the commit 65cbaad to get more accurate results

@@            Coverage Diff             @@
##             main    #1885      +/-   ##
==========================================
+ Coverage   62.87%   64.44%   +1.56%     
==========================================
  Files         133      200      +67     
  Lines       17246    15961    -1285     
==========================================
- Hits        10844    10286     -558     
+ Misses       6402     5675     -727     

Impacted Files	Coverage Δ
astroquery/gemini/__init__.py	100.00% <100.00%> (ø)
astroquery/gemini/core.py	69.56% <100.00%> (-1.13%)	:arrow_down:
astroquery/mast/__init__.py	100.00% <100.00%> (ø)
astroquery/mast/observations.py	75.52% <100.00%> (-0.90%)	:arrow_down:
astroquery/heasarc/core.py	21.48% <0.00%> (-52.31%)	:arrow_down:
astroquery/utils/system_tools.py	52.17% <0.00%> (-31.16%)	:arrow_down:
astroquery/mast/core.py	78.26% <0.00%> (-12.37%)	:arrow_down:
astroquery/gaia/core.py	61.31% <0.00%> (-10.63%)	:arrow_down:
astroquery/esa/hubble/core.py	76.10% <0.00%> (-10.03%)	:arrow_down:
astroquery/utils/url_helpers.py	85.00% <0.00%> (-9.12%)	:arrow_down:
... and 162 more

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[ceb8]

@olyoberdorf You probably want to take a look at this too.

[olyoberdorf]

Gemini is very new, but the deprecation work would be essentially identical to what's needed for MAST. So you may as well do both. Also, thanks for working on my module :)

[bsipocz]

Oh, and as I see there are some conflicts due to a PR that has been merged in the meantime, so this will need a rebase at some point (better to do it once you've done the docs updates, too).

Also, note to maintainers that travis is gone, and I have to fix the CI before we can reasonably merge anything.

[ceb8]

@bsipocz Which of your comments did you need my input on?

Also thanks for remembering the documentation! Super important!

[bsipocz]

This needs a rebase due to conflicts. I may try to get to it before release to make sure the deprecations end up in the release.

[pep8speaks]

Hello @tinumide! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2022-05-05 08:04:42 UTC

[bsipocz]

~Good news, I see the docs build error on this branch locally, too. Bad news, I don't see the reason, the same build passes.~ See inline comment for the fix.

The deprecation of the instances in a way that the warning is not raised at module import time is I suppose the last outstanding issue, but there I don't yet have a solution.

[bsipocz]

And I spotted one more issue, if you do an import the deprecation warnings should not show. I only have an ugly workaround for the class instance and I'm not sure how I could force it to emit a deprecation when used, but not when the module is imported.

Python 3.9.1 (default, Jan 11 2021, 15:50:22) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.20.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import astroquery.mast
WARNING: AstropyDeprecationWarning: The ObservationsClass class is deprecated and may be removed in a future version.
        Use MastObservationsClass instead. [astroquery.mast.observations]

Maybe @pllim you have a nice idea to work this around?

[bsipocz]

I'm sorry, but this has to wait for the next release, I just can't find a way to have all our cases covered, to make sure all users who use the old names to see the warning, but those using the new one don't.

[bsipocz]

Heads up @ceb8 @jaymedina @tomdonaldson @olyoberdorf: I would like to go ahead with this sooner rather than later. Would you be still on board? Basically we already followed the same logic with the new class in mast, calling it e.g. MastMissions.

It is understood that we'll need long deprecation period, for both modules so for a while all the users will experience is a deprecation warning at the time of import.

As I see the last comments in here, this got stalled with some issues around getting the right warning in all situations. Besides fixing that, do you see any blockers with the proposed idea?

[olyoberdorf]

Hi Brigitta,

Yeah still on board. I don’t recall any other problems. I’m on vacation but I am back this weekend.

On Fri, Mar 18, 2022 at 12:23 PM Brigitta Sipőcz @.***> wrote:

Heads up @ceb8 https://github.com/ceb8 @jaymedina https://github.com/jaymedina @tomdonaldson https://github.com/tomdonaldson @olyoberdorf https://github.com/olyoberdorf: I would like to go ahead with this sooner rather than later. Would you be still on board? Basically we already followed the same logic with the new class in mast, calling it e.g. MastMissions.

It is understood that we'll need long deprecation period, for both modules so for a while all the users will experience is a deprecation warning at the time of import.

As I see the last comments in here, this got stalled with some issues around getting the right warning in all situations. Besides fixing that, do you see any blockers with the proposed idea?

— Reply to this email directly, view it on GitHub https://github.com/astropy/astroquery/pull/1885#issuecomment-1072018814, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACKOSDNTXBHFATOFQT4PA2TVAQAMNANCNFSM4TJUHZZA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>