Packagedcode to handle Python(requirements.txt) - #2078
Conversation
588b2f7 to
63a9c30
Compare
steven-esser
left a comment
There was a problem hiding this comment.
There are a few stylistic changes to be made, but more importantly, this code does not work correctly.
See comments below.
63a9c30 to
a5e06c3
Compare
pombredanne
left a comment
There was a problem hiding this comment.
Have you checked dparse too? https://github.com/pyupio/dparse ?
It is using the "official" requirement handler from the "packaging" package https://github.com/pyupio/dparse/blob/master/dparse/parser.py#L15
a5e06c3 to
f552a4f
Compare
|
@MaJuRG @pombredanne |
steven-esser
left a comment
There was a problem hiding this comment.
This is looking pretty good. See one nitpick comment.
f552a4f to
f6bbaa1
Compare
pombredanne
left a comment
There was a problem hiding this comment.
Thank you, we are almost there! 👍
See my comments inline.
Signed-off-by: rpotter12 <rohitpotter12@gmail.com>
Signed-off-by: rpotter12 <rohitpotter12@gmail.com>
f6bbaa1 to
83f29f9
Compare
|
@MaJuRG @pombredanne If everything looks good please approve and merge this PR.:) |
steven-esser
left a comment
There was a problem hiding this comment.
This looks good to me. @pombredanne Anything else to add here?
| parsers = { | ||
| 'setup.py': parse_setup_py, | ||
| 'requirements.txt': parse_requirements_txt, | ||
| 'requirements.in': parse_requirements_txt, |
There was a problem hiding this comment.
Also note that for the future, I have submitted this PR upstream pyupio/dparse#48
Signed-off-by: rpotter12 <rohitpotter12@gmail.com>
pombredanne
left a comment
There was a problem hiding this comment.
Thanks: Just a few last nits and this will be merged!
Signed-off-by: rpotter12 <rohitpotter12@gmail.com>
|
@pombredanne Please review the PR again. :) |
pombredanne
left a comment
There was a problem hiding this comment.
Thank you and I have a few last nits for you!
| return | ||
| file_name = fileutils.file_name(location) | ||
|
|
||
| file_name = get_dependency_type(file_name) |
There was a problem hiding this comment.
may be rather:
dependency_type = get_dependency_type(file_name)
|
|
||
| file_name = get_dependency_type(file_name) | ||
|
|
||
| if file_name not in (filetypes.requirements_txt, |
There was a problem hiding this comment.
if dependency_type not in (filetypes.requirements_txt,
| requirement = None | ||
| if specs: | ||
| requirement = str(specs) | ||
| if '==' in requirement: |
There was a problem hiding this comment.
What about === and others quasi-equal in the latest specs?
Also since df_dependencies.specs is a list of Specifier packaging object https://github.com/pypa/packaging/blob/master/packaging/specifiers.py why not use that instead of taking a str() on it?
You could check if the .operator matches a list of equality operators AND that there is only one specifier (which is the only valid case for is_resolved )
See https://github.com/pypa/packaging/blob/master/packaging/specifiers.py#L173
Signed-off-by: rpotter12 <rohitpotter12@gmail.com>
| name=df_dependency.name | ||
| ).to_string() | ||
| if specs: | ||
| requirement = specs[0] |
There was a problem hiding this comment.
what if there are more than one items in `specs?
There was a problem hiding this comment.
I think you missed adding back something like:
requirement = str(df_dependency.specs)
?
| ).to_string() | ||
| if specs: | ||
| requirement = specs[0] | ||
| if specs[0].operator == '==': |
There was a problem hiding this comment.
what if there are more than one items in specs?
Signed-off-by: rpotter12 <rohitpotter12@gmail.com>
pombredanne
left a comment
There was a problem hiding this comment.
LGTM now... Merging!
Thank you for your efforts there!
Note also this follow up ticket #2114
Fixes #2037
Tasks
Run tests locally to check for errors.