Added support for package-lock.json to parse additional features #3988
Conversation
VarshaUN
force-pushed
the
support-package-lock.json
branch
from
381909f to
71453ba
Compare
There was a problem hiding this comment.
@VarshaUN are you sure what you are doing is correct?
- You are making changes to the package.json parser here, and not package-lock.json which was the target of issue Ensure we can collect the latest package-lock.json including file indirections for versions #3493
- Please add tests for each of the new functionality added so we can easily verify whether the code actually works and produces any useful data, use the github search with paths to look for useful examples.
|
Gentle ping @VarshaUN |
VarshaUN
force-pushed
the
support-package-lock.json
branch
from
ff330fc to
7f1b0de
Compare
…8@gmail.com> Signed-off-by: Varsha U N <varshamaddur2006@gmail.com>
Signed-off-by: Varsha U N <varshamaddur2006@gmail.com>
Signed-off-by: Varsha U N <varshamaddur2006@gmail.com>
VarshaUN
force-pushed
the
support-package-lock.json
branch
from
59db77f to
0632016
Compare
There was a problem hiding this comment.
@VarshaUN thanks for the PR, but I don't think this achieves anything at all.
Please see my comments on that, and also my comment at #3981 (review) which details how to fix the related issue.
|
|
||
| def handle_http_source(self, pkg_path, pkg_data): | ||
| """ Handle HTTP tarball sources. """ | ||
| logging.info(f'Handling HTTP source for {pkg_path}') |
There was a problem hiding this comment.
I don't think these functions are doing anything at all except logging?
Not sure I understand the use of this or how this is related to the issue.
| @@ -804,6 +804,46 @@ class NpmPackageLockJsonHandler(BaseNpmLockHandler): | |||
| description = 'npm package-lock.json lockfile' | |||
| documentation_url = 'https://docs.npmjs.com/cli/v8/configuring-npm/package-lock-json' | |||
|
|
|||
| def parse(self, location): | |||
There was a problem hiding this comment.
Why do you need to define another parse() function here and do nothing except logging? we already have one defined above.
| handler.parse(test_file) | ||
| with open(expected_file) as f: | ||
| expected_data = json.load(f) | ||
| assert handler.packages == expected_data |
There was a problem hiding this comment.
Please see how tests are written for parsing package manifests below, you need to follow the same structure. check_packages_data does all the checking for you, you just need to specify the input/output and parsing function.
| return NpmPackageLockJsonHandler() | ||
|
|
||
| def test_npm_package_lock_json_parse(handler): | ||
| test_file = 'npm/package-lock-v1/package-lock.json' |
There was a problem hiding this comment.
This test already exists at
😅
|
Hey @AyanSinhaMahapatra I have decided to open a new PR on this as I had some technical issues with this PR . I see that I follow your reviews . Thanks! |
Fixes #3493
Tasks
Signed-off-by: Varsha U N varshaun58@gmail.com