-
Notifications
You must be signed in to change notification settings - Fork 172
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
Fix colon encoding in tests again #416
base: main
Are you sure you want to change the base?
Conversation
@matt-phylum @pombredanne These test changes are putting the cart before the horse. Let's first clarify the |
@pombredanne What did you mean by the language @matt-phylum cites at the top: |
This PR is just changing the test case back to be consistent with the existing documentation and other tests. The test used to be this way for 7 years from 2017-11-16 when the test suite was introduced until 2025-02-05. As it currently stands, you cannot pass the tests without a special case to encode The colon character is unambiguous unencoded everywhere. Not the colon separator. Colons only have meaning when used after The wording of these rules has always been problematic. The first rule says that the separator must not be encoded, but that the Compare to the other rules: purl-spec/PURL-SPECIFICATION.rst Lines 234 to 236 in 7f7e82f
This one is technically wrong. There is one time when slashes are ambiguous and do need to be encoded: when they are within the name. However, it follows the same pattern of referencing the character purl-spec/PURL-SPECIFICATION.rst Lines 238 to 241 in 7f7e82f
For these ones it is more obvious what's going on. These tests are in contradiction to the change being reverted: purl-spec/test-suite-data.json Lines 506 to 517 in 7f7e82f
The colon in the qualifier is not encoded. purl-spec/test-suite-data.json Lines 590 to 601 in 7f7e82f
The colon in the name is not encoded. |
About colons, the spec says:
purl-spec/PURL-SPECIFICATION.rst
Lines 228 to 232 in 7f7e82f
Therefore, the colon should not be encoded. I've read #364 and I can't understand the reasoning for changing the unencoded colon to be encoded. #364 also quotes the spec and references another issue, contradicting its own implementation.
9/14 implementations agree that this colon is not to be encoded.