content icon indicating copy to clipboard operation
content copied to clipboard

fix: rename files/slugs with braces

Open nschonni opened this issue 3 years ago β€’ 9 comments

The brace characters can sometimes cause issues for tools that use regex.

nschonni avatar Sep 09 '22 06:09 nschonni

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)

github-actions[bot] avatar Sep 09 '22 06:09 github-actions[bot]

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

teoli2003 avatar Sep 09 '22 11:09 teoli2003

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.

Elchi3 avatar Sep 09 '22 12:09 Elchi3

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.

teoli2003 avatar Sep 09 '22 12:09 teoli2003

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.

nschonni avatar Sep 09 '22 14:09 nschonni

As a note, Wikipedia does allow parentheses in slugs. (Not saying we should too)

teoli2003 avatar Sep 10 '22 06:09 teoli2003

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?

Josh-Cena avatar Sep 10 '22 14:09 Josh-Cena

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)

nschonni avatar Sep 10 '22 15:09 nschonni

πŸš€ 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.

teoli2003 avatar Sep 12 '22 10:09 teoli2003

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

bsmth avatar Nov 24 '22 15:11 bsmth

This pull request has merge conflicts that must be resolved before it can be merged.

github-actions[bot] avatar Jan 17 '23 08:01 github-actions[bot]

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?

bsmth avatar Jan 17 '23 08:01 bsmth

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})`
    );
  }

OnkarRuikar avatar Jan 17 '23 09:01 OnkarRuikar

Is it necessary to also change the slugs?

Yes. yari throws error

Of course, I forgot about that, thanks for clarifying.

bsmth avatar Jan 17 '23 09:01 bsmth

  1. 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:

  1. If the parentheses cause issues, there needs to be a check that prevents these from landing in the repository again.

schalkneethling avatar Jan 23 '23 10:01 schalkneethling

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. :)

OnkarRuikar avatar Jan 23 '23 12:01 OnkarRuikar

Thanks, everyone. Merging this one shortly πŸ‘πŸ»

bsmth avatar Jan 23 '23 17:01 bsmth

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

nschonni avatar Jan 23 '23 21:01 nschonni

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. πŸ™ŒπŸ»

bsmth avatar Jan 25 '23 10:01 bsmth