-
-
Notifications
You must be signed in to change notification settings - Fork 397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
jplhorizons cleanup 2022.05 #2418
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2418 +/- ##
==========================================
- Coverage 62.92% 62.90% -0.02%
==========================================
Files 133 133
Lines 17269 17291 +22
==========================================
+ Hits 10866 10877 +11
- Misses 6403 6414 +11
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
astroquery/jplhorizons/core.py
Outdated
super().__init__() | ||
self.id = id | ||
self.location = location | ||
|
||
self.server_url = conf.horizons_server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently (in main
) the configuration item is accessed in the functions that need the value, which means that changing it at runtime works as expected. With this change it would be accessed only when the HorizonsClass
instance is created, so any changes to the configuration item after that would have no effect. That would be a regression.
da856a5 should not be merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the opening post of #2341 to explain the motivation behind the changes proposed there, bit I'll provide a short summary here.
The current behavior (in main
) works just fine for everyone using the astropy
configuration system, but not all users can be expected to be familiar with it. It would therefore be good if there were instance attributes that could be used as an alternative to the configuration system, but the way da856a5 implements the instance attribute would mean that any changes made to the configuration system after instance creation would have no effect. That would be an obvious regression.
The best way to implement configurable values would be to follow the template given in #2341.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the motivation, I was just confused by the conflicting advice. Regardless, I'll revert this change. Thanks for the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the configuration system needs those clarifications from #2341 (I'll get to the review of it), my point here (in fact not here, but in the new module) was to not hardwire the URL in each and every method of a class, have it set once on the class instance and call it a day for now.
OTOH refactorings could wait for existing modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying.
Looks like this will also help out with #2203 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry not catching this sooner, but strictly speaking, we should deprecate the get_raw_response
kwarg, even as we have a perfectly fine alternative for the same functionality. That would mean leaving most of the code in place, and do the cleanup in the next release (e.g. we don't need to do a long deprecation, but at least one tagged release cycle would be great as the jplhorizons
module is I feel one of the most widely used ones).
To do that you could use the deprecated_removed_argument
from astropy.utils.decorators, some usage of it can be found in astropy core.
In this case it should look like something like this: @deprecated_renamed_argument("get_raw_response", None, since="0.4.7", alternative="async methods")
To keep the work you did, maybe you could cherry-pick the removal commit into a new branch, and remove it with a rebase here, and can reuse that new branch for a future milestone when the actual deprecation gets removed.
Once it's done I'll do a proper review.
4df2333
to
21844a7
Compare
@bsipocz No problem, that's an easy fix. Thanks for the example deprecation. |
query_type = {'OBSERVER': 'ephemerides', | ||
'ELEMENTS': 'elements', | ||
'VECTORS': 'vectors'}[kwargs['params']['EPHEM_TYPE']] | ||
|
||
if ('TLIST' in kwargs['params']): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ('TLIST' in kwargs['params']): | |
if 'TLIST' in kwargs['params']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
astroquery/jplhorizons/core.py
Outdated
pass | ||
raise | ||
|
||
self.raw_response = response.text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should deprecate this one as well, is it used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good to go if CI is all green
Thanks @mkelley! |
Awesome! |
A limited clean up of docstrings and the
_parse_response
method. Also, remove theget_raw_response
optional argument as this can be done with the*_async()
methods.