-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
Conversation
Codecov Report
@@ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
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, |
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.
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, |
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.
same, make these keyword only, otherwise this is a backward incompatible API change
Thank you @weaverba137! |
This PR closes #2516.
Specifically the methods
get_images(_async)
andget_spectra(_async)
haveget_query_payload
restored.All other methods already fully support
get_query_payload
exceptget_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.