Add ESO catalogue functions#3531
Conversation
ashleythomasbarnes
changed the title
| eso | ||
| ^^^ | ||
|
|
||
| - Add functionality to list and query ESO catalogues. [#8] | ||
|
|
There was a problem hiding this comment.
I seem to remember that this file is normally edited by the astroquery maintainers
There was a problem hiding this comment.
Nope, this is correct, preferably the changelog entry is added in the PR by the PR author.
There was a problem hiding this comment.
Let me know if you need more here...
|
General comment: it’s a bit difficult to focus on the new feature since the PR also contains many formatting changes. Splitting these into a separate commit or PR would make the review easier. |
bsipocz
left a comment
bsipocz
left a comment
There was a problem hiding this comment.
I've started reviewing this, but the code style changes are a bit overwhelming, so I stopped when I got to the actual changes in core.py.
Could you please remove any of the style changes (and anyway, please don't force short line length on code) and force push back the actual code changes?
There was a problem hiding this comment.
This file is not needed any more, please remove.
| tap_url = _config.ConfigItem( | ||
| tap_obs_url = _config.ConfigItem( |
There was a problem hiding this comment.
this is technically an API change; but for now it is acceptable as we haven't yet released the ESO refactor work.
astroquery/eso/core.py
Outdated
| from ..exceptions import ( | ||
| RemoteServiceError, | ||
| LoginError, | ||
| NoResultsWarning, | ||
| MaxResultsWarning, | ||
| ) | ||
| from ..query import QueryWithLogin | ||
| from ..utils import schema | ||
| from .utils import _UserParams, raise_if_coords_not_valid, _reorder_columns, \ | ||
| _raise_if_has_deprecated_keys, _build_adql_string, \ | ||
| DEFAULT_LEAD_COLS_PHASE3, DEFAULT_LEAD_COLS_RAW | ||
| from .utils import ( | ||
| _UserParams, | ||
| raise_if_coords_not_valid, | ||
| _reorder_columns, | ||
| _raise_if_has_deprecated_keys, | ||
| _build_adql_string, | ||
| DEFAULT_LEAD_COLS_PHASE3, | ||
| DEFAULT_LEAD_COLS_RAW, | ||
| ) |
There was a problem hiding this comment.
I really wish these type of style refactorings would not be mixed into feature branches.
Also, we do allow longer lines in astroquery, up to 120 characters, so please don't apply these black type spreads over many lines.
|
Indeed this should have been a draft PR like we agreed (sorry @szampier)... How should we proceed - shall I revert to draft and make changes, or okay to keep as-in? |
|
Okay, I cleaned up my mess - it should be clearer now. Let me know how you want to proceed... |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3531 +/- ##
==========================================
- Coverage 72.71% 72.69% -0.02%
==========================================
Files 219 219
Lines 20473 20543 +70
==========================================
+ Hits 14886 14934 +48
- Misses 5587 5609 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Just to let you know @bsipocz - I spoke with @juanmcloaiza and @szampier this morning, and we're happy for you to continue the review. I will be away from 1st March until ~ 7th April, so it would be great if we could get this wrapped up by then, otherwise @juanmcloaiza has offered to take over any needed changes whilst I'm gone. Thanks! |
3 participants
We've added functions in the same structure similar to the others to query the ESO catalogues:
Docs are coming in another PR.
-- Thanks!
@juanmcloaiza @szampier @almicol