Remove defaults for SIAv1 query params #367
Conversation
tomdonaldson
requested review from
andamian,
cbanek,
jcsegovia and
msdemlei
pyvo/dal/sia.py
Outdated
| @@ -46,7 +46,7 @@ | |||
|
|
|||
|
|
|||
| def search( | |||
| url, pos, size=1.0, format='all', intersect="overlaps", verbosity=2, | |||
| url, pos, size=1.0, format=None, intersect=None, verbosity=None, | |||
There was a problem hiding this comment.
you'll need to update the default for format in the docstring below, too. The rest looks good.
There was a problem hiding this comment.
Oops, good point! There are 2 other places in the code where the docstring mentions the default, but the function signature default is None. I could edit them all to indicate that the default is a server-side default, but then for consistency I suppose I should add that same info for intersect and verbosity. It may help a user to know the server-side default, so maybe it's worth the extra words, but I'm not sure.
There was a problem hiding this comment.
format and intersect are fine as those are the service defaults as well so behaviour is unchanged.
I couldn't find a default for verbosity in the spec so I'm not sure if this changes the behaviour. But the spec says that "Services that do not support this parameter must permit it to be present without error." so it shouldn't create any problems if present.
There was a problem hiding this comment.
You're right about verbosity. I was remembering that it defaults to 2 for Simple Cone Search. To minimize the potential changes in behavior to standard-compliant services for this change, I should restore the client-side default for verbosity.
Ideally (if all services were fully compliant) none of this should matter, but there's a non-compliant service I want to use that breaks if you specify format. I don't want to have special code in pyvo for non-compliant services, but having pyvo specify defaults that should be implemented on the server really is unnecessary at best and possibly confusing. I think the user should have control over what params get submitted in the query.
There was a problem hiding this comment.
The client can work around the above problem with format=None. That's somewhat workable, but would need to be done as a special case for the offending service.
Codecov Report
@@ Coverage Diff @@
## main #367 +/- ##
==========================================
- Coverage 79.97% 79.83% -0.14%
==========================================
Files 52 52
Lines 5972 5972
==========================================
- Hits 4776 4768 -8
- Misses 1196 1204 +8
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
I'd say this is a good idea in principle. I don't think I'm too enthused that we don't use the opportunity to drop the default on VERB, which I think as been a mistake to begin with, in particular because I don't think common validators bother to test that services actually ignore it.
On the other hand, I'd expect that if there are SIA services out there that do anything at all with VERB (which I doubt), passing 2 to them shouldn't hurt, and those that stutter when you pass VERB need to be fixed anyway. Hence: You've got my vote.
|
@tomdonaldson I think this would need a change log. I'm wondering why the bot hasn't picked it up. |
|
the changelog bot is rather flaky, and not always triggered. The issue is known, but we don't yet have a solution for it (or do we @pllim?) |
|
(and due to that flakyness I very much still like using the changelog checking scripts at release time, but then those rely on everything is merged with a merge commit rather than squashed at the top of |
|
If you are talking about astropy-bot , I do not have a solution. It works when it wants to. It is deployed on Heroku and Heroku clamped down on OSS support due to "abuse" (wish I kept link to their blog about that but I don't have it anymore). |
|
I have https://github.com/pllim/action-check_astropy_changelog . Even though it is archived, it still seems to work for Jdaviz. https://github.com/spacetelescope/jdaviz/blob/main/.github/workflows/changelog_check.yml And I just stumbled upon a really basic check that does not require any custom action here if this is good enough for you: https://github.com/spacetelescope/jwst/blob/master/.github/workflows/changelog.yml |
|
On Tue, Jan 24, 2023 at 02:37:19PM -0800, Brigitta Sipőcz wrote:
@bsipocz commented on this pull request.
What about, for good measures, doing the same for SSA, too, where `format='all'` is set?
Excellent point. +1 from me.
|
bsipocz
force-pushed
the
remove_sia_defaults
branch
from
04d6b0a to
00c4c39
Compare
|
Removed the default for SSA, and rebased for changelog conflict. Given the approval, if CI is happy, I'll go ahead and merge. |
|
Thank you @tomdonaldson! |
Remove defaults for SIAv1 query params
Fixes #366
Remove the SIAv2 client-side defaults for the
format,intersectandverbosityparameters. This enables the client to not submit optional parameters for non-compliant services that have trouble with those parameters.