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.
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.
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.
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.
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.
Thanks for clarifying.
|
Looks like this will also help out with #2203 |
There was a problem hiding this comment.
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.
mkelley
force-pushed
the
jplhorizons-cleanup-2022.05
branch
from
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.
| if ('TLIST' in kwargs['params']): | |
| if 'TLIST' in kwargs['params']: |
astroquery/jplhorizons/core.py
Outdated
| pass | ||
| raise | ||
|
|
||
| self.raw_response = response.text |
There was a problem hiding this comment.
I think we should deprecate this one as well, is it used anywhere?
|
Thanks @mkelley! |
|
Awesome! |
A limited clean up of docstrings and the
_parse_responsemethod. Also, remove theget_raw_responseoptional argument as this can be done with the*_async()methods.