Skip to content
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

ESA Integral Science Legacy Archive Astroquery module #3154

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

jespinosaar
Copy link
Contributor

@jespinosaar jespinosaar commented Dec 18, 2024

Dear Astroquery team,

This PR aims to integrate a new module for the ESA Integral Science Legacy Archive. The main difference with other modules is that this is the first integration of an ESA module using PyVO, instead of the deprecated astroquery.utils.tap module. We have developed a fully functional module that can be used as reference to migrate the rest. Please let us know your thoughts and we will iterate on this to merge it. Thanks for your support!

Would it be possible to create a release, right after this module has been merged? Thank you very much.

Kind regards,

@jespinosaar

cc @esdc-esac-esa-int

@pep8speaks
Copy link

pep8speaks commented Dec 18, 2024

Hello @jespinosaar! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-12-19 08:43:26 UTC

@jespinosaar jespinosaar self-assigned this Dec 18, 2024
@jespinosaar
Copy link
Contributor Author

Ok, the tests are not working in the CI/CD environment. It seems that, at some point, it tries to connect to the URL. I would need some support on this. Maybe I should define the tap parameter as a property, similar to what is done for other modules. What do you think, @bsipocz ? Do you recommend any other alternative?

@jespinosaar jespinosaar force-pushed the ESA_isla-create-astroquery branch from e9d45f3 to 8368fd6 Compare December 19, 2024 08:41
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

Attention: Patch coverage is 82.57426% with 88 lines in your changes missing coverage. Please review.

Project coverage is 67.94%. Comparing base (cc279d5) to head (af9d8e6).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
astroquery/esa/utils/utils.py 59.86% 59 Missing ⚠️
astroquery/esa/integral/core.py 91.39% 29 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3154      +/-   ##
==========================================
+ Coverage   67.54%   67.94%   +0.39%     
==========================================
  Files         233      235       +2     
  Lines       18493    18998     +505     
==========================================
+ Hits        12491    12908     +417     
- Misses       6002     6090      +88     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jespinosaar
Copy link
Contributor Author

I applied the changes I commented before and it seems all checks are passing now! So we can now check if everything is ok, iterate on changes and, when possible, merge it. Please let me know your thoughts and thanks in advance for the feedback!

@bsipocz
Copy link
Member

bsipocz commented Dec 19, 2024

Thanks @jespinosaar. I'll bump this into the new year as it's unrealistic to promise a review before we're off for a break.

@bsipocz bsipocz added this to the v0.4.9 milestone Dec 19, 2024
@jespinosaar
Copy link
Contributor Author

Of course, no problem! thanks @bsipocz and Merry Christmas!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants