Deprecate raw response from non-async method#3089
Conversation
2024 AA does not return uncertainty info anymore. Rather than continuously chase objects with uncertianties, just test mocked data.
|
Thanks! This change brings mpc more in line with the standard astroquery interface. 👍 from me. |
bsipocz
left a comment
There was a problem hiding this comment.
Some minor comments, mostly to do the deprecations properly (at least for one version cycle), and to clarify a bit around the test changes.
And while you're doing generic cleanup and maintenance, I wonder if you would mind having a look at the failing doctests in the narrative documentation, too. (pytest docs/mpc -R should do the trick of running them)
| return self._request('GET', mpc_endpoint, params=request_args, auth=auth) | ||
|
|
||
| def get_mpc_object_endpoint(self, target_type): | ||
| def _get_mpc_object_endpoint(self, target_type): |
There was a problem hiding this comment.
this is technically an API change (though wasn't used in the docs, nor has any docstrings in the API docs), so I would say, mentioning the cleanup the changelog would be sufficient.
| perturbed=True, unc_links=False, | ||
| get_query_payload=False, | ||
| get_raw_response=False, cache=False): | ||
| get_query_payload=False, cache=False): |
There was a problem hiding this comment.
changelog says deprecate, yet outright removed. Consider adding the decorator for now.
There was a problem hiding this comment.
This one was removed rather than deprecated because the parameter was unused and had no effect on the method's behavior. I can deprecate it even though it doesn't do anything, or just add a comment in the change log.
There was a problem hiding this comment.
The same comment for get_observatory_codes.
There was a problem hiding this comment.
Mentioned these changes (and justification) in the change log.
| return request_args | ||
|
|
||
| @class_or_instance | ||
| @deprecated_renamed_argument("get_raw_response", None, since="0.4.9", |
There was a problem hiding this comment.
This is what needs to be done for the other methods, too.
| @deprecated_renamed_argument("get_raw_response", None, since="0.4.9", | |
| @deprecated_renamed_argument("get_raw_response", None, since="0.4.8", |
| alternative="async methods") | ||
| def get_observations_async(self, targetid, *, | ||
| id_type=None, | ||
| comettype=None, |
There was a problem hiding this comment.
what happened with this parameter?
There was a problem hiding this comment.
It was an unused parameter and doesn't correspond to anything in the API. I can make a comment in the change log.
There was a problem hiding this comment.
Mentioned in the change log.
| result = mpc.core.MPC.get_ephemeris('2P', location='G37') | ||
| assert result['Moon phase'][0] >= 0 | ||
|
|
||
| def test_get_ephemeris_Uncertainty(self): |
There was a problem hiding this comment.
What are the reason for removing these? I see that you mention to cleanup some tests to only run the mock versions but these seem to be removed for the mock test, too.
There was a problem hiding this comment.
The uncertainty column is effectively tested with the test_get_ephemeris_unc_links method. Perhaps the method should be renamed.
There was a problem hiding this comment.
This is the remote file, and these are definitely the tests that need to be removed.
| assert result['Moon phase'][0] >= 0 | ||
|
|
||
|
|
||
| def test_get_ephemeris_Uncertainty(patch_post): |
There was a problem hiding this comment.
what are the reason for removing these tests?
There was a problem hiding this comment.
Ah, this is a mistake. I thought I had made a copy-paste error, but these were actually working mocked tests. I can restore them.
There was a problem hiding this comment.
I partially thought it was a mistake because test_get_ephemeris_Moon_phase_and_Uncertainty was defined twice.
I think PR #3026 attempted to replace 1994 XG with 2024 AA, even though in the mocked tests it wasn't needed (because the mocked data didn't change, only the online results). I'll clean this up.
|
I've edited the doctest, too. It should now pass. |
Instead, direct users to use the async method.