-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add a option for local search to fido client #143
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #143 +/- ##
==========================================
+ Coverage 73.56% 75.31% +1.75%
==========================================
Files 32 32
Lines 2016 2111 +95
==========================================
+ Hits 1483 1590 +107
+ Misses 533 521 -12 ☔ View full report in Codecov by Sentry. |
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.
Looks ok will need a change log e.g. https://github.com/TCDSolar/stixpy/blob/main/changelog/README.rst
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.
I think we should switch to anc, asp, ephemeris for level, datatype and product so remove aux etc?
I don't think we need to add so many files I know they are empty but should be able to just mock the list of file names similar to here but for a local client https://github.com/sunpy/radiospectra/blob/main/radiospectra/net/sources/tests/test_rstn_client.py
Also need to think about the versions thing what happens if you search for both min and latest specifically how if works with https://docs.sunpy.org/en/latest/topic_guide/extending_fido.html#search-attrs
It may be better as function which operates on the Fido.search
result?
Not sure if the test fail is real or just a typo?
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.
Some (trivial) regex comments/suggestions
stixpy/net/client.py
Outdated
ql_filename = r"solo_{level}_stix-{product}_[0-9]{{8}}_V[0-9]{{2}}[a-zA-Z]?.fits" | ||
sci_filename = ( | ||
r"solo_{level}_stix-{product}_" r"[0-9]{{8}}T[0-9]{{6}}-[0-9]{{8}}T[0-9]{{6}}_V[0-9]{{2}}[a-zA-Z]?_.*.fits" | ||
) |
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.
- Did the changes to
ql_filename
ansci_filename
actually affect matching? Your commit message implies that you made these changes for Windows, but I don't immediately see how that would be the case. - The period in
.fits
should formally be escaped
Co-authored-by: Albert Y. Shih <[email protected]>
Co-authored-by: Shane Maloney <[email protected]>
Co-authored-by: Shane Maloney <[email protected]>
Co-authored-by: Shane Maloney <[email protected]>
Co-authored-by: Shane Maloney <[email protected]>
No description provided.