Move cloud functions into Observations, fix for docs and test

[snbianco]

Closes #3097

After talking with others at MAST, it looks like enable_cloud_dataset and disable_cloud_dataset are only applicable for Observations and no other classes (as of now). To minimize complexity, I decided to move the functions into Observations so as to not make them available to other classes that inherit from MastQueryWithLogin. This fixes the documentation issue as well.

There was also a problem with one of our tests that I fixed, so the test suite is passing now.

[codecov]

Codecov Report

Attention: Patch coverage is 60.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 67.49%. Comparing base (58f8692) to head (4e422d8).
Report is 24 commits behind head on main.

Files with missing lines	Patch %	Lines
astroquery/mast/core.py	60.00%	2 Missing ⚠️
astroquery/mast/observations.py	60.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3113   +/-   ##
=======================================
  Coverage   67.49%   67.49%           
=======================================
  Files         233      233           
  Lines       18413    18420    +7     
=======================================
+ Hits        12428    12433    +5     
- Misses       5985     5987    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[snbianco]

Do the code coverage checks only use the tests in test_mast.py? Is that why they're failing?

[bsipocz]

I would say these should be deprecated rather than starting to break user code without notice; while these have no effect, the code do run with non-cloud returns, right?

The deprecations would work nicely if you keep overriding the methods in the classes that these work and not get the deprecation.

What do you think @pllim?

[pllim]

Yes, I think these should be deprecated first. It sounds like people might be using these in the wild whether intentionally or not.

[snbianco]

This makes sense! The latest version deprecates the functions in the base class.

[pllim]

Thanks! Hmm you could also use @deprecated decorator from astropy.utils but I'll leave that to astroquery maintainers to decide.

[snbianco]

What's your preference @bsipocz? It looks like there are instances of both methods in the repo.

[bsipocz]

we mostly use the decorator for methods/arguments/etc., most of the direct raise of the warnings are for cases when a parameter value or a full module is affected.

[snbianco]

Made that change! I followed the convention of the other deprecated functions and left the original docstrings in, but let me know if something else would be preferable.

[bsipocz]

(minor note: could you also mast label these PRs at opening? Also, can add the next milestone, there should be only one open, for any of the PRs.
That would help with triaging.)

[pep8speaks]

Hello @snbianco! 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! 🍻

Comment last updated at 2024-10-17 21:44:46 UTC

[snbianco]

(minor note: could you also mast label these PRs at opening? Also, can add the next milestone, there should be only one open, for any of the PRs. That would help with triaging.)

For sure, I'll remember this for future PRs!

[bsipocz]

OK, this looks good. I'll rebase for merging though (please update the PR with a rebase instead of a merge commit next time).