-
Notifications
You must be signed in to change notification settings - Fork 76
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
gsoc: package.json info extraction #118
gsoc: package.json info extraction #118
Conversation
|
Are you duplicating this 6 commits in all the PRs? That is bad, we request change in one PR and you'll have to rebase all the PRs, why not get the base merged first? 😕 |
I know it's bad, but keeping in mind the deadlines both have to be worked upon simultaneously |
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 alright except for a few comments here and there
|
||
|
||
class VersionInfo(Info): | ||
description = "Version range information." |
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.
version range? Like pip>=0.7.3
? or an actual list of versions?
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.
It'd be really neat to have an example value as a docstring for each type of Info
|
||
|
||
class ManPathsInfo(Info): | ||
description = "Related to man-pages" |
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.
Does this mean actual man-page archive file paths or just the term?
|
||
|
||
class PackageJSONInfoExtractor(InfoExtractor): | ||
supported_file_globs = ("package.json",) |
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.
quick question: the .
in the glob isn't "match any character", right? Do we need to escape it? I don't quite remember how our globbing works :D
The simplest way to test would be to create a file called "packagexjson" and see if it's picked up.
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.
.
matches only .
, just verified :)
packagexjson
fails for glob "package.json"
and package.json
falis for escaped glob "package/.json"
VersionInfo) | ||
|
||
|
||
class PackageJSONInfoExtractor(InfoExtractor): |
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.
Could you include a link to something like a RFC/spec defining what goes into a package.json file? Same thing for all other files too. It'd be neat to have a reference link right in the code.
ManPathsInfo) | ||
|
||
def parse_file(self, fanme, file_content): | ||
return json.loads(file_content) |
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 can possibly throw exceptions if the input is malformed; can we wrap it around a try-except and return {}
if there's some error?
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.
and add a test that verifies this scenario
@@ -27,9 +27,11 @@ def find_information(self, fname, parsed_file): | |||
class InfoA(Info): | |||
description = 'Information A' | |||
value_type = (str, int) | |||
example_values = ['coala', 420] |
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.
hehe
@rultor merge |
@adtac @satwikkansal Oops, I failed. You can see the full log here (spent 2min)
|
Adds a class attribute named spec_references which contains links to the issues/documentations for relevant specs of supported files. Related to #126
Adds a class attribute named example_values illustrating sample information values that are allowed to be stored inside the Info class. Closes #125
Adds Info classes relevant to extractors of package.json and other meta-files. Related to #100
Adds ``PackageJSONInfoExtractor`` class to extract relevant information from package.json file. Closes #100
@rultor merge |
No description provided.