-
-
Notifications
You must be signed in to change notification settings - Fork 573
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
Implemented Kanzelhohe Client #3810
Conversation
Copying over my two relevant comments from #3763:
Originally posted by @ayshih in #3763 (comment)
Originally posted by @ayshih in #3763 (comment) |
I will change the name of the client, as per reviewers' instructions; after they all decide. |
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 refactoring suggestions
c9dfffa
to
93a42a9
Compare
@dpshelio did all the suggested changes. The failing tests are from HEK(not from the changes I made), maybe go away by rerunning the tests. |
da6be72
to
fa4b5b0
Compare
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.
All looks good @abhijeetmanhas - let's see if someone that knows German or from KSO comments on the ö/o/oe problem.
__all__ = ['KanzelhoheClient'] | ||
|
||
|
||
class KanzelhoheClient(GenericClient): |
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.
This is more tricky... if we don't put ö
on the class name, should we put oe
?
@segfaulthunter what would you say?
@dpshelio @nabobalis for naming, can we just have |
You mean call it the KANZClinet? |
yep |
Sure. |
6eefeff
to
e742e0b
Compare
from sunpy.net import Fido | ||
import sunpy.net.dataretriever.sources.kanzelhohe as kanzelhohe | ||
|
||
KClient = kanzelhohe.KANZClient() |
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.
This should be turned into a pytest fixture.
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.
Done
KClient.search(TRANGE, a.Wavelength(6000 * u.AA)) | ||
|
||
|
||
# This test downloads 3 files |
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.
Is it possible to download 1 file?
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.
Fixed.
Hello @abhijeetmanhas! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-05-08 18:16:38 UTC |
Split of PR #3763 .