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

fix: rename files/slugs with parens #20470

Merged
merged 3 commits into from
Jan 23, 2023
Merged

fix: rename files/slugs with parens #20470

merged 3 commits into from
Jan 23, 2023

Conversation

nschonni
Copy link
Contributor

@nschonni nschonni commented Sep 9, 2022

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

@nschonni nschonni requested review from a team as code owners September 9, 2022 06:33
@nschonni nschonni requested review from teoli2003 and estelle and removed request for a team September 9, 2022 06:33
@github-actions github-actions bot added Content:CSS Cascading Style Sheets docs Content:Glossary Glossary entries Content:HTTP HTTP docs labels Sep 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2022

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)

caugner
caugner previously requested changes Sep 9, 2022
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

This PR is removing parentheses from all URLs.

Two thoughts:
1.This requires approval by @Rumyra and possibly OWD (cc @Elchi3).
2. If the parentheses cause issues, there needs to be a check that prevents these from landing in the repository again.

I think it should be possible to fix the issues without having to rename the pages. But if folks were planning on the those braces anyways, let's do it.

@teoli2003
Copy link
Contributor

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

@Elchi3
Copy link
Member

Elchi3 commented Sep 9, 2022

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.

@teoli2003
Copy link
Contributor

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.

@nschonni
Copy link
Contributor Author

nschonni commented Sep 9, 2022

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.

@@ -3626,7 +3628,7 @@
/en-US/docs/Glossary/Web_Sockets /en-US/docs/Glossary/WebSockets
/en-US/docs/Glossary/World_Wide_Web_Consortium_(W3C) /en-US/docs/Glossary/W3C
/en-US/docs/Glossary/XForm /en-US/docs/Glossary/XForms
/en-US/docs/Glossary/XMLHttpRequest /en-US/docs/Glossary/XHR_(XMLHttpRequest)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it looks like this is going back to the previous naming, I wonder if an extra redirect for the a plain /XHR and the other acronyms that are being dropped would make sense

@teoli2003
Copy link
Contributor

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

@Josh-Cena
Copy link
Member

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?

@nschonni
Copy link
Contributor Author

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)

@teoli2003
Copy link
Contributor

🚀 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.

Copy link
Contributor Author

@nschonni nschonni left a comment

Choose a reason for hiding this comment

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

Add back removed metadata spacing

files/en-us/web/css/_colon_-moz-locale-dir_ltr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/_colon_-moz-locale-dir_rtl/index.md Outdated Show resolved Hide resolved
files/en-us/web/http/cross-origin_resource_policy/index.md Outdated Show resolved Hide resolved
@caugner caugner requested a review from Rumyra October 24, 2022 12:48
@nschonni nschonni force-pushed the brace-files branch 2 times, most recently from 1160b3e to e1ef02e Compare October 25, 2022 17:35
@bsmth
Copy link
Member

bsmth commented Nov 24, 2022

Thanks all, I've opened a discussion so we can come to an agreement on how to move forward here: mdn/mdn-community#293

@bsmth bsmth added the on hold Waiting on something else before this can be moved forward. label Nov 24, 2022
@github-actions
Copy link
Contributor

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

@bsmth
Copy link
Member

bsmth commented Jan 17, 2023

Minor conflicts after #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?

@OnkarRuikar
Copy link
Contributor

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

@bsmth
Copy link
Member

bsmth commented Jan 17, 2023

Is it necessary to also change the slugs?

Yes. yari throws error

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

Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Going to leave a +1 on this. Would be good to get another approval from the team before merging (and resolving conflicts).

@schalkneethling
Copy link
Contributor

  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.

@OnkarRuikar
Copy link
Contributor

OnkarRuikar commented Jan 23, 2023

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:

- name: Check changed files
if: ${{ env.GIT_DIFF_FILES }}
run: |
echo ${{ env.GIT_DIFF_FILES }}
export CONTENT_ROOT=$(pwd)/files
yarn filecheck ${{ env.GIT_DIFF_FILES }}

We can do it only after this PR lands. :)

@bsmth
Copy link
Member

bsmth commented Jan 23, 2023

Thanks, everyone. Merging this one shortly 👍🏻

@bsmth bsmth dismissed caugner’s stale review January 23, 2023 17:23

CI check for parens in file paths taken care of in follow-up

@Josh-Cena Josh-Cena changed the title fix: rename files/slugs with braces fix: rename files/slugs with parens Jan 23, 2023
@bsmth bsmth merged commit 3624c20 into mdn:main Jan 23, 2023
@nschonni nschonni deleted the brace-files branch January 23, 2023 17:39
@nschonni
Copy link
Contributor Author

FYI, 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

@bsmth
Copy link
Member

bsmth commented Jan 25, 2023

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 #23859 which seems to have landed before yours. 🙌🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:CSS Cascading Style Sheets docs Content:Glossary Glossary entries Content:HTTP HTTP docs on hold Waiting on something else before this can be moved forward.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants