Skip to content
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

URLManifestItem.url shouldn't contain non-URL-unit characters #181

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gsnedders
Copy link
Member

@gsnedders gsnedders changed the title Add RFC XXX - URLManifestItem.url shouldn't contain non-URL-unit characters URLManifestItem.url shouldn't contain non-URL-unit characters Feb 16, 2024
@gsnedders
Copy link
Member Author

(There's also the variant of this RFC where all we do is url.replace(" ", "%20") which avoids the "space separated URLs" problem. But it still carries the same risks.)

rfcs/valid-item-urls.md Outdated Show resolved Hide resolved
Co-authored-by: Ms2ger <Ms2ger@gmail.com>
@foolip
Copy link
Member

foolip commented Feb 17, 2024

One question here is whether we should disallow test file paths that contain spaces, or whether we should just escape these in the URL.

Given how few such files there are, let's just disallow them.

Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storing URLs (and therefore test ids) in url-encoded form does make sense.

Migrating expectation data is a challenge. I wonder if for the wpt expectation files (which handle spaces fine) we could just unencode the test id before using the manifest? That makes it more likely that the manifest will match whatever people write in the <meta name=varaint>. Alternatively if we're going to change to using the URL encoded form everywhere, I think we should have a lint to ensure we use it consistently (maybe we should have a lint either way, so that we don't get %20 in some places and in another).

I don't think banning spaces in test names is worthwhile.

@foolip
Copy link
Member

foolip commented May 14, 2024

I don't think banning spaces in test names is worthwhile.

I just searched for spaces in filenames and found two cases that don't seem intentional:

web-platform-tests/wpt#46253
web-platform-tests/wpt#46254

Is there much of a downside to banning spaces?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants