-
-
Notifications
You must be signed in to change notification settings - Fork 641
Parse .npmrc files
#4608
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
base: develop
Are you sure you want to change the base?
Parse .npmrc files
#4608
Conversation
) Add NpmrcHandler in src/packagedcode/npm.py to parse .npmrc configuration files and yield a PackageData with parsed key/value pairs in extra_data. Add unit test and fixtures under tests/packagedcode/data/npm/basic/. Signed-off-by: Shekhar <[email protected]>
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.
Thanks @levi42x see comments for your consideration. A bit of research into fields could be nice to see if we can use anything to populate the package-data fields, see https://docs.npmjs.com/cli/v11/using-npm/config#config-settings for all the fields and check out real world usages for these config values.
| @@ -0,0 +1,15 @@ | |||
| import os | |||
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.
No need to create a new file, add the tests in the test_npm.py file
| class TestNpmrc(PackageTester): | ||
| test_data_dir = os.path.join(os.path.dirname(__file__), 'data') | ||
|
|
||
| def test_parse_basic_npmrc(self): |
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.
also add a test to check npm.NpmrcHandler.is_datafile() works as expected
| @@ -0,0 +1,8 @@ | |||
| ; sample .npmrc for tests | |||
| # a comment line | |||
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.
Please use real .npmrc files seen in the wild like with: https://github.com/search?q=path%3A*.npmrc&type=code
This also helps you looks for what kind of data is present and whether we can use these differently rather than just storing in the extra_data
| key, value = line.split('=', 1) | ||
| key = key.strip() | ||
| value = value.strip() | ||
| # ignore empty key but allow empty values |
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.
are there cases with empty values that is still useful to keep? can you provide examples?
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.
thanks for pointing it out.
i did not even think about it at that time..
i'm not able to find any empty value (in key value pair) that could be useful.
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.
yet i have not seen any empty values case, should i include it to handler or just skip it (example: registry= ) ?
| parse [.npmrc] file and store result in key : value pair. | ||
| convert key : value pair to object and return it. | ||
| """ | ||
| extra_data = {} |
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.
There are .npmrc examples with license/author info which we need to parse and store properly, this is not just extra data.
Please also research examples found in the wild/docs to see what other fields we can use like this to map useful data to package data fields.
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'll update the handler to map meaningful fields (license, author, homepage, etc.) to ScanCode’s structured package fields.
and other config-only fields (like proxy, cafile, always-auth) in extra_data, since they don’t map to package metadata.
| default_package_type = 'npm' | ||
| default_primary_language = None | ||
| description = 'npm .npmrc configuration file' | ||
| documentation_url = 'https://docs.npmjs.com/cli/v11/configuring-npm/npmrc' |
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.
| documentation_url = 'https://docs.npmjs.com/cli/v11/configuring-npm/npmrc' | |
| documentation_url = 'https://docs.npmjs.com/cli/configuring-npm/npmrc' |
this would pin the link to this version and will soon be outdated. I see some other npm doc URLs also have versions, can you also remove those in the PR?
| for member in workspace_members: | ||
| member.save(codebase) | ||
|
|
||
| class NpmrcHandler(BaseNpmHandler): |
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 part of the npm assembly only if the data present in .npmrc files can be used to updated package data information in other npm manifests. The registry information present could be used to update npm urls in npm packages found alongside these manifests but these URLs are not always usable. And you'd have to modify the BaseNpmHandler.assemble function to handle/use these too.
So let's keep this as a subclass of models.NonAssemblableDatafileHandler to avoid any assembly from these files and keep things simple for now.
NpmrcHandlerto enable parsing of.npmrcfiles.init.*defaults) and stores them inextra_dataonPackageData, making npm configuration discoverable through ScanCode outputs.This PR includes the following updates:
NpmrcHandlerinnpm.pytest_npmrc.py.npmrctest fixture and its expected result fileFixes #4494
Tasks
CHANGELOG.rst(can be added upon request)Signed-off-by: Shekhar Suman [email protected]