-
-
Notifications
You must be signed in to change notification settings - Fork 423
Addresses Issue #3138 - ESO - Enable Table Access Protocol (TAP) queries #3339
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
base: main
Are you sure you want to change the base?
Conversation
720fbd9
to
22d0c30
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3339 +/- ##
==========================================
+ Coverage 70.54% 71.16% +0.61%
==========================================
Files 232 233 +1
Lines 20022 19963 -59
==========================================
+ Hits 14125 14206 +81
+ Misses 5897 5757 -140 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a987745
to
4b775f8
Compare
Hi @bsipocz , could you please give us a time estimate of the review process? No pressure at all though, we only need to plan our activities internally. Thanks a lot in advance. |
@juanmcloaiza - sorry for the delay. I'll try to get to the review this week. |
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 to have this many comments and thank you for your patience while waiting for this review.
I acknowledge that this seems to a lot, but most of them are minor and/or helping API consistency with the other modules and/or making the code cleaner and easier to read and follow; which will be helpful for the future maintenance.
3742bb9
to
d3be2af
Compare
@bsipocz, thanks a lot for your comments, I have addressed most of them (the easy ones); for the rest, I'd still need some feedback from you. If you can give me feedback today, I can work immediately on it and push and update by tomorrow. Afterwards, I won't be able to work on Astroquery for the next couple of weeks. Just so you know, in case you can squeeze this review in your day 🤓 Thanks again! 👍 |
@juanmcloaiza - I'm sorry, it's very unlikely that I can get back to this today, but I'll try to do it soon so you can pick it up again in a few weeks time. |
Hi @bsipocz , just a ping about the second review round. Have you had some time for it? We're looking forward to hearing back from you. |
f4a18a6
to
9d713ee
Compare
There's no field for me to reply to your las two comments (strange...), but here are my replies: About the maxrec - Sure thing, About the deprecated keys message - The problem is that the astroquery ESO module (like many others) is currently using the keywords of the underlying querying engine directly in its API (like The messages users get are, for example, Without @ashleythomasbarnes has already an extensive documentation with examples ready to be submitted after this PR. For the moment, the changes to the documentation in this PR, include a warning and a how-to regarding the incompatibility between WDB and TAP:
Thanks for the feedback, @keflavich, I'll get everything done this week. Please just let me know about my question regarding the deprecation of arguments; for the rest everything is clear 👍 |
@keflavich, your comments have been taken into account, thanks for having taken the time. [cc @bsipocz, @ashleythomasbarnes, @szampier]. -- EDIT: Apologies, I went again one by one through all the conversations and I noticed three comments that had escaped my sight. I have taken them into account now. Please double check and let me know if anything is still missing. |
Closes #3138
Changes:
Switch querying interface from WDB to TAP in querying functions:
Deprecate arguments:
open_form
cache
New functionality:
query_tap_service
(with authentication as well)cone_ra
,cone_dec
,cone_radius
argumentscount_only
argument