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
Added method to extract metadata from url using parse #4313
Conversation
Hello @abhijeetmanhas! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2020-06-26 11:57:56 UTC |
Two tests are failing which are in extern parse code. |
Need to exclude the entire extern location from pytest in setup.cfg. I would have thought it would have been. |
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 this is good, just a few API design questions.
sunpy/util/scraper.py
Outdated
@@ -331,3 +332,42 @@ def _smallerPattern(self, directoryPattern): | |||
return None | |||
except Exception: | |||
raise | |||
|
|||
def _extractMetaURLs(self, timerange, extractor=None, translator=None): |
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 function name isn't great, python method names are supposed to use snake_case.
I am not sure what a good name for this function is, it's doing a few things (calling filelist as well as running things through parse). Do we want to see this become the main entry point for scraper in a land where you use parse for everything?
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.
Yes, filelist
will become internal function for scraper and the client would only be using the extractMetaURLs
method.
if you pull in master the figure test build should pass. |
ea3fb9d
to
759f09a
Compare
Can you rebase this onto master? |
Windows failures unrelated.
|
Doc failure is due to
which is unrelated to the PR. |
Description
It is a split of #4213 in which a new method is added for scraper changes which would possibly be backward compatible.