-
Notifications
You must be signed in to change notification settings - Fork 197
Updated CPAN type spec #420
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: main
Are you sure you want to change the base?
Conversation
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 good! Just a few tweaks, and one question.
Would it make sense to add some failing tests?
e.g. the previous format with Module::Names
, and perhaps a couple more uses, with a short description on why it is invalid?
@giterlizzi Thanks. We need to make sure we are all clear first wrt. the encoding in the core spec and that is getting close to merge ready in #398 |
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.
the PR now includes these new tests for the new CPAN specification:
- valid CPAN purl
- CPAN distribution in BackPAN repository
- invalid CPAN distribution name as module name
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 good to me, with a few final optional suggestions.
test-suite-data.json
Outdated
"version": null, | ||
"qualifiers": null, | ||
"subpath": null, | ||
"is_invalid": false | ||
"is_invalid": true | ||
}, |
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.
Would it be sensible to add a few more tests for corner cases?
- Distribution name with lowercase characters
- Distribution name which contains
::
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 otherwise good! :-)
PURL-TYPES.rst
Outdated
- The ``name`` is the module or distribution name and is case sensitive. | ||
- The ``version`` is the module or distribution version. | ||
- The ``namespace`` is the CPAN id of the author/publisher. It MUST be written uppercase and is required. | ||
- The ``name`` is the distribution name and is case sensitive. |
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.
Not sure, but would it make sense to still point out that…
A distribution name MUST NOT contain the string
::
.
…as earlier? I get it that it's strictly speaking not necessary, but as a hint for new users to understand the difference between a module name and a distribution name, I think it can be helpful.
(I have no strong opinions on this idea, and leave it to you to decide if this goes in or not)
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 agree, adding this sentence (A distribution name MUST NOT contain the string ::
) reinforces the concept that only the distribution MUST be used.
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.
"name" component description updated
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.
Looking good, and this is a big change... could it make sense to also add a FAQ entry and give hints to implementers what they can do when parsing older ::
PURLs?
After the merge of PR #514, PURL types are now defined in JSON: 😇 😁 With the new approach... this PR needs to be updated. Could you look into this? Thanks for your understanding! |
@sjn @pombredanne Hi, i have updated the CPAN type spec and tests. |
If @sjn agrees, I will add the "notes" field with this sentence: The previous CPAN PURL type specification allowed module names (e.g. URI::PackageURL) to be used as PURL "names" while also omitting the PURL "namespace". The parser MUST emit an error when a module is specified as a PURL "name" or detect "::" characters. |
Added "note" about the previous CPAN specification
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.
@giterlizzi this looks all good to me... is there more that you need to add to this?
This PR is for the new version of the purl-type CPAN specification and includes new updated tests.
giterlizzi/perl-URI-PackageURL#8
In brief, this PR confirms the rules for generating a PURL string for CPAN.
Previously it was possible to create PURL strings in two ways:
Using only the module could create ambiguity, because in CPAN a distribution can have several authors (and co-authors) over time and does not allow precise identification of the artifact.
It was clarified that the author of the "distribution" is mandatory, allowing an artifact and its URL to be identified along with its version.