-
Notifications
You must be signed in to change notification settings - Fork 12
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
npm package names should not be lowered #19
npm package names should not be lowered #19
Conversation
npm used to support package names containing capital characters: https://blog.npmjs.org/post/168978377570/new-package-moniker-rules.html Such packages still exist on the npm registry, like this one: https://www.npmjs.com/package/Acid/v/3.0.17
Hey @am11, mind taking a look at this? This bug is preventing our system from handling older npm packages properly, so we would love to get a fix in and a new version published to NuGet sooner rather than later. |
{ | ||
"nuget" or "cocoapods" or "cpan" or "vsm" or "cran" => name, | ||
"nuget" or "cocoapods" or "cpan" or "vsm" or "cran" or "npm" => name, | ||
"pypi" => name.Replace('_', '-').ToLower(), | ||
_ => name.ToLower() |
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.
Should we align name and namespace validations with python implementation: https://github.com/package-url/packageurl-python/blob/6c96667/src/packageurl/__init__.py#L94? i.e. ToLower() transformation is not applied unless we are sure it's necessary.
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.
Great point - I'd be happy to contribute that next week, if you don't mind that going in as a separate PR.
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.
Sorry, I missed it earlier. 😄
LGTM, other than a question/comment; which can be tackled in a separate PR.
I think it'd be good to align the normalization logic across the organization, and have some tests in shared test data.
Thanks @am11! Aligning to preserve-case-by-default is a great idea - I'd be happy to contribute that next week. In the mean time, it would be helpful if we could be unblocked by getting this patch checked in and published - would you mind publishing this patch in a new version to NuGet? Thanks for your time looking at this. |
Oops, I clicked the wrong button I think 😅 |
v1.1.1 is published: https://www.nuget.org/packages/packageurl-dotnet/1.1.1 Thank you for the contribution. 👍 |
Like a few other package managers, npm package names are case sensitive. They now disallow uppercase characters in names for new packages (as discussed here), but older packages having uppercase names still exist (see https://www.npmjs.com/package/Acid vs. https://www.npmjs.com/package/acid).
So far I haven't seen evidence that scopes having uppercase characters exist, so I left that as-is.