Move cloud functions into Observations, fix for docs and test#3113
Conversation
Codecov ReportAttention: Patch coverage is
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. |
|
Do the code coverage checks only use the tests in |
| self._auth_obj.logout() | ||
| self._authenticated = False | ||
|
|
||
| def enable_cloud_dataset(self, provider="AWS", profile=None, verbose=True): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, I think these should be deprecated first. It sounds like people might be using these in the wild whether intentionally or not.
There was a problem hiding this comment.
This makes sense! The latest version deprecates the functions in the base class.
There was a problem hiding this comment.
Thanks! Hmm you could also use @deprecated decorator from astropy.utils but I'll leave that to astroquery maintainers to decide.
There was a problem hiding this comment.
What's your preference @bsipocz? It looks like there are instances of both methods in the repo.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
(minor note: could you also |
For sure, I'll remember this for future PRs! |
0a2f14c to
6e49df3
Compare
51447d9 to
ea176a0
Compare
bsipocz
left a comment
There was a problem hiding this comment.
OK, this looks good. I'll rebase for merging though (please update the PR with a rebase instead of a merge commit next time).
fix function docstrings
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
ea176a0 to
4e422d8
Compare
Closes #3097
After talking with others at MAST, it looks like
enable_cloud_datasetanddisable_cloud_datasetare only applicable forObservationsand no other classes (as of now). To minimize complexity, I decided to move the functions intoObservationsso as to not make them available to other classes that inherit fromMastQueryWithLogin. 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.