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

Helper method for checking against required and optional query attrs #3979

Merged
merged 2 commits into from
Apr 9, 2020

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented Apr 3, 2020

The logic for checking a query in jsoc seems quite common, and I need to use similar logic in the SPDF client I am writing. It seems sensible to put it in a helper method. Questions

I'm not sure what it should be called? (simple_can_handle_query is a bit of a rubbish name...)

@dstansby dstansby added this to the 2.0 milestone Apr 3, 2020
@dstansby dstansby requested a review from a team as a code owner April 3, 2020 12:48
@dstansby dstansby added the net Affects the net submodule label Apr 3, 2020
@nabobalis
Copy link
Contributor

What about query_handler?

@Cadair
Copy link
Member

Cadair commented Apr 6, 2020

I think this would be useful as a helper as a part of a more complex _can_handle_query, i.e. you could call this first and then check specific values of attrs later having verified the types are there with this, so I think the name should reflect that as well. (Also I think this pattern might be useable in some of the dataretriever clients as well, maybe you could check?)

So maybe something like check_attr_types_in_query?

@dstansby dstansby force-pushed the simple-query branch 3 times, most recently from f53f5dd to 1e07f78 Compare April 6, 2020 11:47
@nabobalis nabobalis merged commit c79af34 into sunpy:master Apr 9, 2020
@dstansby dstansby deleted the simple-query branch April 9, 2020 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Affects the net submodule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants