content
content copied to clipboard
fix: rename files/slugs with braces
The brace characters can sometimes cause issues for tools that use regex.
Preview URLs (6 pages)
Flaws (5)
Note! 3 documents with no flaws that don't need to be listed. π
URL: /en-US/docs/Web/CSS/:-moz-locale-dir_ltr
Title: :-moz-locale-dir(ltr)
Flaw count: 2
- macros:
/en-US/docs/Web/CSS/:-moz-locale-dir(rtl) redirects to /en-US/docs/Web/CSS/:-moz-locale-dir_rtl
- broken_links:
Can't resolve /en-US/docs/Archive/Mozilla/XUL
URL: /en-US/docs/Web/CSS/:-moz-locale-dir_rtl
Title: :-moz-locale-dir(rtl)
Flaw count: 2
- macros:
/en-US/docs/Web/CSS/:-moz-locale-dir(ltr) redirects to /en-US/docs/Web/CSS/:-moz-locale-dir_ltr
- broken_links:
Can't resolve /en-US/docs/Making_Sure_Your_Theme_Works_with_RTL_Locales
URL: /en-US/docs/Glossary/Round_Trip_Time
Title: Round Trip Time (RTT)
Flaw count: 1
- broken_links:
Can't resolve /en-US/docs/Glossary/time_to_first_byte
(comment last updated: 2023-01-23 10:57:01)
The issue of this morning has been fixed without removing the braces. Onkar made the workflow more resilient.
I have no strong feeling about keeping them or removing them
1.This requires approval by @Rumyra and possibly OWD (cc @Elchi3).
I'm not aware of anything that would depend on the braces (although you never know what is hidden in all the weird sidebar implementations π )
I think it makes sense to only allow unproblematic ("friendly") URLs for MDN pages (and in mdn translations). I don't know if yari has a check for which slugs it allows but there are probably best practices out there, see e.g. https://stackoverflow.com/questions/695438/what-are-the-safe-characters-for-making-urls. So, it might nice to be quite strict here from the yari side.
There are a few other restrictions (like no accented characters) that we hit a long time ago (with BΓ©zier curve generating a flaw in the dashboard). And like the : and @ characters (that are forbidden by HTTP if I recall well) that are replaced by _colon_ and _at_ in MDN filenames.
The issue of this morning has been fixed without removing the braces. Onkar made the workflow more resilient.
No it's still an issue, but is no longer failing the build. These files with braces in the name are just silently failing to be linteted, as Markdownlint treats them as a regex and fails to match them.
As a note, Wikipedia does allow parentheses in slugs. (Not saying we should too)
I have a slight preference to keeping parentheses in slugs (mostly because Wikipedia does that as well). Maybe we can remove them from file paths?
I think Wikipedia uses them for disambiguation. I think the only one that falls into that bucket might be Descriptor (CSS). The rest are adding the acronyms after the word, or leftover parts of syntax braces (which were removed everywhere else)
π Automation π leads to many surprises, especially on such a large repository! π We are learning little by little how to do it optimally. That's great. π
I think there should be a discussion at https://github.com/mdn/mdn-community/discussions before going forward.
Especially, I think the following points should be presented:
- What characters to exclude and why (forbidden by standard, making automation complicated because they have a special meaning in shells, risk of spoofing, β¦)
- What conventions do other large websites have.
- What has to be updated (# pages to rename, meta-docs to update, scripts to guarantee the problem doesn't come back, β¦).
I don't have time to open this myself (especially as I'll be away for a few weeks) but feel free to start it. (I don't have a strong opinion here so I won't come bikeshedding after the facts)
Meanwhile, we maybe want to transform this PR into a draft to show it is not ready to move forward.
Thanks all, I've opened a discussion so we can come to an agreement on how to move forward here: https://github.com/mdn/mdn-community/discussions/293
This pull request has merge conflicts that must be resolved before it can be merged.
Minor conflicts after https://github.com/mdn/content/pull/23602 has landed.
Regarding the changes, I am +1 on at least renaming the files, i.e. the following looks fine to me:
/glossary/descriptor_(css)/index.md->/glossary/css_descriptor/index.md
Is it necessary to also change the slugs?
Is it necessary to also change the slugs?
Yes. yari throws error:
if (Document.urlToFolderPath(document.url) !== document.fileInfo.folder) {
throw new Error(
`The document's slug (${metadata.slug}) doesn't match its disk folder name (${document.fileInfo.folder})`
);
}
Is it necessary to also change the slugs?
Yes. yari throws error
Of course, I forgot about that, thanks for clarifying.
- If the parentheses cause issues, there needs to be a check that prevents these from landing in the repository again.
Looks good to me, and it seems that we have reached a consensus that we want to do this. My only question is whether we want, need, or have addressed this point from Claas:
- If the parentheses cause issues, there needs to be a check that prevents these from landing in the repository again.
My only question is whether we want, need, or have addressed this point from Claas:
@schalkneethling ATM markdownlint pr check fails when a file name has (. But we need to have explicit check and error message for it.
I think better place to handle this would be in yari file checker i.e. on yari side. We already have a workflow that executes this command:
https://github.com/mdn/content/blob/fd75bb869943f4c9a87a57a5723625476441fe66/.github/workflows/pr-test.yml#L135-L141
We can do it only after this PR lands. :)
Thanks, everyone. Merging this one shortly ππ»
FYI, hit a minor regression with Yari, so sent in https://github.com/mdn/content/pull/23849 for the Kitchen Sink tests. As far as I can tell, it just wasn't expecting a redirect on that test page
Hit a minor regression with Yari, so sent in #23849 for the Kitchen Sink tests. As far as I can tell, it just wasn't expecting a redirect on that test page
Thanks a lot! Linking to https://github.com/mdn/content/pull/23859 which seems to have landed before yours. ππ»