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

Restore get_query_payload to all methods #2532

Merged
merged 3 commits into from
Sep 17, 2022

Conversation

weaverba137
Copy link
Member

This PR closes #2516.

Specifically the methods get_images(_async) and get_spectra(_async) have get_query_payload restored.

All other methods already fully support get_query_payload except get_spectral_template(_async), which never supported it, as far as I can tell. That method works significantly differently from all other methods, so it is not surprising that it does not have that capability.

Unit tests are included to verify that the query payload really is returned by these updated methods.

@codecov
Copy link

codecov bot commented Sep 16, 2022

Codecov Report

Merging #2532 (ab8d5b8) into main (4362450) will increase coverage by 0.01%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##             main    #2532      +/-   ##
==========================================
+ Coverage   63.06%   63.07%   +0.01%     
==========================================
  Files         133      133              
  Lines       17322    17333      +11     
==========================================
+ Hits        10924    10933       +9     
- Misses       6398     6400       +2     
Impacted Files Coverage Δ
astroquery/sdss/core.py 91.61% <89.47%> (-0.38%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

Minor backwards API related comments (I'll push a quick commit for these so a merge can go ahead without either of us coming back to it next week).

Thanks!

@@ -520,7 +520,7 @@ class = 'galaxy' \

def get_spectra_async(self, coordinates=None, radius=2. * u.arcsec,
matches=None, plate=None, fiberID=None, mjd=None,
timeout=TIMEOUT,
timeout=TIMEOUT, get_query_payload=False,
Copy link
Member

Choose a reason for hiding this comment

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

the order of the kwargs are different than in the previous release (before the previous PR). So while it's not strictly related, making all optional arguments keyword only is the way to go here.

@@ -640,8 +650,8 @@ def get_spectra_async(self, coordinates=None, radius=2. * u.arcsec,
@prepend_docstr_nosections(get_spectra_async.__doc__)
def get_spectra(self, coordinates=None, radius=2. * u.arcsec,
matches=None, plate=None, fiberID=None, mjd=None,
timeout=TIMEOUT, cache=True,
data_release=conf.default_release,
timeout=TIMEOUT, get_query_payload=False,
Copy link
Member

Choose a reason for hiding this comment

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

same, make these keyword only, otherwise this is a backward incompatible API change

@bsipocz bsipocz added this to the v0.4.7 milestone Sep 17, 2022
@bsipocz bsipocz merged commit f6799dc into astropy:main Sep 17, 2022
@bsipocz
Copy link
Member

bsipocz commented Sep 17, 2022

Thank you @weaverba137!

@bsipocz bsipocz mentioned this pull request Nov 26, 2022
62 tasks
@weaverba137 weaverba137 deleted the restore-query-payload branch February 10, 2023 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SDSS: add get_query_payload kwarg back to all methods
2 participants