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

BUG: Fix issues related to kwarg only changes in utils.tap #2892

Merged
merged 6 commits into from
Dec 8, 2023

Conversation

cosmoJFH
Copy link

@cosmoJFH cosmoJFH commented Dec 7, 2023

In the issue #2884, it was detected a problem related to the function Gaia.launch_job.

It looks like this is related to the commit

Tsar Bomba Nick 18/3/23 1:15 Refactor: Made kwargs keyword only for astroquery/utils/tap/tapconn.py
rev number: e724f62

where the the function in astroquery/utils/tap/tapconn.py were made kwargs keyword

We have analysed the changes carried out in that commit, and more classes are affected.

cc @esdc-esac-esa-int

@cosmoJFH cosmoJFH changed the title Fix issues related to changes in commit e724f62de444de23aa7ad88aac19b… for tapconn.py Fix issues related to changes in commit e724f62de444de23aa7ad88aac19b… for tapconn.py and other classes Dec 7, 2023
@jespinosaar
Copy link
Contributor

Thanks @cosmoJFH , I am currently testing it in other ESA modules, so maybe more changes are required.

Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (885734b) 66.53% compared to head (06b2619) 66.53%.
Report is 8 commits behind head on main.

Files Patch % Lines
astroquery/utils/tap/core.py 0.00% 10 Missing ⚠️
astroquery/utils/tap/conn/tapconn.py 0.00% 3 Missing ⚠️
astroquery/utils/tap/model/job.py 0.00% 2 Missing ⚠️
astroquery/esa/jwst/core.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2892   +/-   ##
=======================================
  Coverage   66.53%   66.53%           
=======================================
  Files         235      235           
  Lines       18114    18116    +2     
=======================================
+ Hits        12052    12054    +2     
  Misses       6062     6062           

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

@bsipocz
Copy link
Member

bsipocz commented Dec 7, 2023

@cosmoJFH - Thank you for the PR. I'll hold-off merging until @jespinosaar gives the thumbs up that all is good with the rest of the ESA modules.

I'm sorry for not catching these in the review cycle of #2690

@bsipocz
Copy link
Member

bsipocz commented Dec 7, 2023

(I also wonder, how and why we didn't see more failures from all of these before, none of them are covered sufficiently with tests?)

@bsipocz bsipocz changed the title Fix issues related to changes in commit e724f62de444de23aa7ad88aac19b… for tapconn.py and other classes BUG: Fix issues related to kwarg only changes in utils.tap Dec 7, 2023
@jespinosaar
Copy link
Contributor

jespinosaar commented Dec 8, 2023

Ok, just checked and included some changes for the rest of ESA modules. Now it is ok from my side, @bsipocz . And thanks @cosmoJFH for taking care of this so fast!

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you both of you for the very quick bugfix cycle!

@bsipocz bsipocz merged commit 684e46e into astropy:main Dec 8, 2023
10 of 11 checks passed
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.

Gaia.launch_job does not work when uploading tables on the fly
3 participants