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

enhance(filecheck): validate all file paths for lowercase and special characters #8019

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

OnkarRuikar
Copy link
Contributor

@OnkarRuikar OnkarRuikar commented Jan 23, 2023

Addresses mdn/content#20470 (comment)

Ban parentheses from content file paths.


The PR also fixes currently on going erroneous message: Error: files/en-us/web/http/cookies/cookie-basic-example.drawio file-type could not be extracted at all (probably not a .drawio file)
This is because the code considers .drawio as a binary file.

@OnkarRuikar OnkarRuikar changed the title filecheck: ban parentheses filecheck: validate all file paths for lowercase and special characters Jan 23, 2023
@caugner caugner self-requested a review January 30, 2023 13:35
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.

LGTM, very good initiative, thanks! 👏

Just some nits and possible improvements:

filecheck/checker.ts Outdated Show resolved Hide resolved
filecheck/checker.ts Outdated Show resolved Hide resolved
filecheck/checker.ts Outdated Show resolved Hide resolved
filecheck/checker.ts Outdated Show resolved Hide resolved
filecheck/checker.ts Outdated Show resolved Hide resolved
filecheck/checker.ts Outdated Show resolved Hide resolved
filecheck/checker.ts Outdated Show resolved Hide resolved
filecheck/checker.ts Outdated Show resolved Hide resolved
filecheck/checker.ts Outdated Show resolved Hide resolved
filecheck/checker.ts Outdated Show resolved Hide resolved
@caugner caugner changed the title filecheck: validate all file paths for lowercase and special characters enhance(filecheck): validate all file paths for lowercase and special characters Jan 31, 2023
@caugner caugner marked this pull request as draft February 1, 2023 10:24
@github-actions github-actions bot added dependencies Pull requests that update a dependency file merge conflicts 🚧 Please rebase onto or merge the latest main. labels Feb 2, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

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

@github-actions github-actions bot removed dependencies Pull requests that update a dependency file merge conflicts 🚧 Please rebase onto or merge the latest main. labels Feb 2, 2023
@OnkarRuikar
Copy link
Contributor Author

Made small fix in compression logic. Below image is getting compressed to bigger size: 😕

error: Error: ../content/files/en-us/web/api/htmlimageelement/sizes/new-york-skyline-wide.jpg is too large (443.9KB > 500.0KB), even after compressing to 512.7KB.

new-york-skyline-wide

filecheck/checker.ts Outdated Show resolved Hide resolved
filecheck/checker.ts Outdated Show resolved Hide resolved
filecheck/checker.ts Outdated Show resolved Hide resolved
filecheck/checker.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot added the merge conflicts 🚧 Please rebase onto or merge the latest main. label Feb 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

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

@github-actions github-actions bot removed the merge conflicts 🚧 Please rebase onto or merge the latest main. label Feb 8, 2023
@OnkarRuikar OnkarRuikar marked this pull request as ready for review February 20, 2023 08:20
@caugner caugner removed their request for review May 2, 2023 08:19
@caugner caugner requested a review from a team as a code owner November 7, 2023 19:24
@github-actions github-actions bot added the idle label Dec 15, 2023
@github-actions github-actions bot removed the idle label Jan 8, 2024
@github-actions github-actions bot added the idle label Feb 7, 2024
@OnkarRuikar OnkarRuikar requested a review from caugner May 8, 2024 04:10
@github-actions github-actions bot removed the idle label May 8, 2024
Copy link
Contributor

@fiji-flo fiji-flo left a comment

Choose a reason for hiding this comment

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

@OnkarRuikar thanks a lot for this. And sorry for not getting to it in time. I left some inline comments. But to sum it up. As soon as the minor regressions are fixed this is great and ready to go.

filecheck/checker.ts Outdated Show resolved Hide resolved
filecheck/checker.ts Outdated Show resolved Hide resolved
filecheck/checker.ts Outdated Show resolved Hide resolved
@yin1999
Copy link
Member

yin1999 commented Jun 9, 2024

Cross reference: #10859

I wonder if we should only use characters that won't be URL encoded

@OnkarRuikar
Copy link
Contributor Author

I wonder if we should only use characters that won't be URL encoded

I've addressed this in the code:

 // Use only ASCII characters
  const normalized = shortPath.normalize("NFD");
  if (shortPath !== normalized) {
    errors.push(
      `Error: Invalid path: ${filePath}. Use only plain ASCII characters.`
    );
  }

At the moment it logs:

error: Error: Invalid path: /home/onkar/GitHub/mdn/translated-content/files/fr/glossary/bezier_curve/bézier_2_big.gif. Use only plain ASCII characters.
       Error: Invalid path: /home/onkar/GitHub/mdn/translated-content/files/ru/learn/html/introduction_to_html/getting_started/безымянный.png. Use only plain ASCII characters.
       Error: Invalid path: /home/onkar/GitHub/mdn/translated-content/files/ja/orphaned/mozilla/add-ons/webextensions/debugging_(before_firefox_50)/index.md. File path must not include characters: '(', ')'
       FixableError: /home/onkar/GitHub/mdn/content/files/en-us/web/css/css_containment/container_queries/container-query.svg is 52.4KB and can be compressed to 38.4KB (27%)

@OnkarRuikar
Copy link
Contributor Author

@fiji-flo there is one more issue about filecheck tool. When content of an index.md is updated and surrounding image is no longer used in the content the image becomes orphan current tool doesn't report it on index.md modifications. The tool needs to look in the parent directory of index.md for images and check them as well. Should we fix this in this PR or handle it in a new one?

@caugner
Copy link
Contributor

caugner commented Jun 27, 2024

The tool needs to look in the parent directory of index.md for images and check them as well. Should we fix this in this PR or handle it in a new one?

Sorry for the late reply, I would recommend to open a separate PR.

@OnkarRuikar
Copy link
Contributor Author

I would recommend to open a separate PR.

can we merge this now then? 😃

@github-actions github-actions bot added the idle label Jul 28, 2024
@github-actions github-actions bot removed the idle label Aug 16, 2024
@github-actions github-actions bot added the idle label Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants