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

Clean up query code #47

Merged
merged 2 commits into from
Sep 7, 2022
Merged

Clean up query code #47

merged 2 commits into from
Sep 7, 2022

Conversation

dstansby
Copy link
Member

As suggested by @ebuchlin in #32 (comment)

@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2022

Codecov Report

Base: 97.63% // Head: 97.58% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (3c49299) compared to base (40db8e2).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #47      +/-   ##
==========================================
- Coverage   97.63%   97.58%   -0.05%     
==========================================
  Files           4        4              
  Lines         211      207       -4     
==========================================
- Hits          206      202       -4     
  Misses          5        5              
Impacted Files Coverage Δ
sunpy_soar/client.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

tap_endpoint = 'http://soar.esac.esa.int/soar-sl-tap/tap'
payload = SOARClient._construct_payload(query)
# Need to force requests to not form-encode the paramaters
payload = '&'.join([f'{key}={val}' for key, val in payload.items()])

Choose a reason for hiding this comment

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

Ah, if you need this, doesn't this defeat the purpose of having the payload as a dict? What is form-encoded that should not be form-encoded? I guess the '+'s from the ADQL query, but these could be spaces in the first place (then encoded to '+'s; for example, I have code in which the ADQL query is built by f"SELECT filename FROM v_sc_data_item WHERE (instrument='SPICE') AND (insertion_time>='{date_range_str[0]}') AND (insertion_time<='{date_range_str[1]}')"). Anything else?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, unfortunately I think spaces or + characters are form encoded, so neither works. I think this PR is still an improvement though, so thanks for suggesting :)

Copy link
Member

Choose a reason for hiding this comment

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

I think using urllib.parse.urlencode rather than doing this manually would be a small improvement.

@dstansby dstansby requested a review from a team September 2, 2022 21:03
@Cadair Cadair self-requested a review September 6, 2022 10:16
@dstansby
Copy link
Member Author

dstansby commented Sep 7, 2022

Given this has an approval and I think it's an improvement, I'm going to merge - if anyone wants to improve further, please open a new PR 😃

@dstansby dstansby merged commit 091cf32 into main Sep 7, 2022
@dstansby dstansby deleted the query-clean branch September 7, 2022 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants