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

chore: add lint autofix CI job #9604

Merged
merged 5 commits into from
Dec 1, 2023
Merged

chore: add lint autofix CI job #9604

merged 5 commits into from
Dec 1, 2023

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Dec 1, 2023

Motivation

It is very time-consuming in the long run to fix lint issues in internal/external PRs

This job should do it automatically by commiting to the PRs to fix

Regarding spelling, it is even more painful to manage the project-words.txt list manually, so let's move to a simpler workflow where we don't manage that list ourselves, but only review the generated one in PRs: after all we want to be noticed when an unknown word is introduced, and this can be done by reviewing an automated addition to the project-words.txt file.

Test Plan

CI

@slorber slorber added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Dec 1, 2023
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Dec 1, 2023
Copy link

netlify bot commented Dec 1, 2023

[V2]

Name Link
🔨 Latest commit 3ede3fb
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/656a20944956de0008f7a489
😎 Deploy Preview https://deploy-preview-9604--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

github-actions bot commented Dec 1, 2023

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 74 🟢 98 🟢 100 🟢 100 🟠 89 Report
/docs/installation 🟠 66 🟢 98 🟢 100 🟢 100 🟠 89 Report
/docs/category/getting-started 🟠 76 🟢 100 🟢 100 🟢 90 🟠 89 Report
/blog 🟠 75 🟢 100 🟢 100 🟢 90 🟠 89 Report
/blog/preparing-your-site-for-docusaurus-v3 🟠 64 🟢 97 🟢 100 🟢 100 🟠 89 Report
/blog/tags/release 🟠 76 🟢 100 🟢 100 🟠 80 🟠 89 Report
/blog/tags 🟠 76 🟢 100 🟢 100 🟢 90 🟠 89 Report

Copy link

netlify bot commented Dec 1, 2023

[V2]

Name Link
🔨 Latest commit c08f4c9
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/656a219c9412c50008639104
😎 Deploy Preview https://deploy-preview-9604--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

github-actions bot commented Dec 1, 2023

Size Change: 0 B

Total Size: 926 kB

ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 58.9 kB
website/build/assets/css/styles.********.css 114 kB
website/build/assets/js/main.********.js 715 kB
website/build/index.html 37.6 kB

compressed-size-action

"lint:spelling": "cspell \"**\" --no-progress",
"lint:js:fix": "yarn lint:js --fix",
"lint:spelling": "cspell \"**\" --no-progress --show-context --show-suggestions",
"lint:spelling:fix": "yarn rimraf project-words.txt && echo \"# Project Words - DO NOT TOUCH - This is updated through CI\" >> project-words.txt && yarn -s lint:spelling --words-only --unique --no-exit-code --no-summary \"**\" | sort --ignore-case >> project-words.txt",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should not "autofix" spelling. This is not safe and hard to review. For example, words with different capitalization will now show up as separate entries.

@Josh-Cena
Copy link
Collaborator

I can see the point of auto-applying linting/prettier fixes (although I don't find it too painful), but auto-fixing spelling seems too risky.

it is even more painful to manage the project-words.txt list manually

Is it? For me personally: I use my editor to transform everything to lowercase and then sort it alphabetically. I'm aware that alphabetical order may be locale-dependent, but can we simply have a script that does this using Intl.Collator("en-US") to iron out the discrepancies?

@slorber
Copy link
Collaborator Author

slorber commented Dec 1, 2023

I can see the point of auto-applying linting/prettier fixes (although I don't find it too painful)

I spend a lot of time fixing patch-x PRs of external contributors, particularly for docs. This is a real pain for me to not being able to merge half of good-enough docs PRs immediately.

#9490

https://github.com/facebook/docusaurus/actions/runs/6721876263/job/18268564837?pr=9490

Prettier/CI doesn't even tell you the problem:

CleanShot 2023-12-01 at 19 55 20@2x

So I have to load it locally and fix it every time.

And then need to wait for the CI to complete again before merging, which keeps my brain busy and context switch again for no good reason.


Regarding project-words.txt, similarly external contributors are not aware of it, it's basically impossible to find what to fix for someone unaware, and I have to also do this workflow.

We should not "autofix" spelling. This is not safe and hard to review. For example, words with different capitalization will now show up as separate entries.

The goal is not to autofix spelling but to be aware when there's an unknown word.

I review all PRs carefully, including this file, and don't see how it's less safe than having an external contributor adding it by themselves: we don't care if it's someone else or CI that modifies the file, we always already have to review it.

The point is to be "aware" that something is happening here, and any diff to that file is enough for me.

The order, case sensitiveness, deduplication doesn't matter much to me as long as I can review a diff. Actually I want to be aware when see "webpack" and "Webpack", that means we have an inconsistent usage of that lib name.


Not even counting the time lost due to using precommit hooks.
After years of using them, I kind of agree with this take and would rather remove them all:
https://www.youtube.com/watch?v=RAelLqnnOp0


So, for now I'll merge this and we'll see how it goes in practice. Happy to revisit but I'm confident it will be way better than it used to be.

@slorber slorber merged commit 7650829 into main Dec 1, 2023
2 checks passed
@slorber slorber deleted the slorber/autofix-action branch December 1, 2023 19:09
slorber added a commit that referenced this pull request Jan 5, 2024
slorber added a commit that referenced this pull request Jan 5, 2024
Co-authored-by: Joshua Chen <sidachen2003@gmail.com>
Co-authored-by: sebastienlorber <lorber.sebastien@gmail.com>
Co-authored-by: Sébastien Lorber <slorber@users.noreply.github.com>
Co-authored-by: Ivan Mar (sOkam!) <7308253+heysokam@users.noreply.github.com>
Co-authored-by: c0h1b4 <dwidman@gmail.com>
Co-authored-by: Janessa Garrow <janessa.garrow@gmail.com>
Co-authored-by: ozaki <29860391+OzakIOne@users.noreply.github.com>
Co-authored-by: axmmisaka <6500159+axmmisaka@users.noreply.github.com>
Co-authored-by: Tatsunori Uchino <tats.u@live.jp>
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
fix(i18n): complete translations for theme-common.json Brazilian Portuguese (pt-BR) (#9477)
fix(content-blog): add baseUrl for author.image_url (#9581)
fix(type-aliases): add `title` prop for imported inline SVG React components (#9612)
fix(utils): Markdown link replacement with <> but no spaces (#9617)
fix(live-codeblock): stabilize react-live transformCode callback, fix editor/preview desync (#9631)
fix(cli): output help when no conventional config + no subcommand (#9648)
fix CI job (#9604)
fix Lint Autofix workflow (#9632)
fix(pwa-plugin): upgrade workbox (#9668)
fix(create-docusaurus): fix init template code blocks, and little improvements (#9696)
fix(theme): allow empty code blocks and live playgrounds (#9704)
slorber added a commit that referenced this pull request Jan 26, 2024
Co-authored-by: Joshua Chen <sidachen2003@gmail.com>
Co-authored-by: Joey Clover <joey@popos.local>
Co-authored-by: reece-white <93522192+reece-white@users.noreply.github.com>
Co-authored-by: Shreesh Nautiyal <114166000+Shreesh09@users.noreply.github.com>
Co-authored-by: Nick Gerleman <nick@nickgerleman.com>
Co-authored-by: Chongyi Zheng <git@zcy.dev>
Co-authored-by: MCR Studio <99176216+mcrstudio@users.noreply.github.com>
Co-authored-by: sebastienlorber <lorber.sebastien@gmail.com>
Co-authored-by: Sébastien Lorber <slorber@users.noreply.github.com>
Co-authored-by: Ivan Mar (sOkam!) <7308253+heysokam@users.noreply.github.com>
Co-authored-by: c0h1b4 <dwidman@gmail.com>
Co-authored-by: Janessa Garrow <janessa.garrow@gmail.com>
Co-authored-by: ozaki <29860391+OzakIOne@users.noreply.github.com>
Co-authored-by: axmmisaka <6500159+axmmisaka@users.noreply.github.com>
Co-authored-by: Tatsunori Uchino <tats.u@live.jp>
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
Co-authored-by: Sanjaiyan Parthipan <parthipankalayini@gmail.com>
Co-authored-by: Jack Robson <143492403+jack-robson@users.noreply.github.com>
Co-authored-by: dawei-wang <dawei-wang@users.noreply.github.com>
Co-authored-by: eitsupi <50911393+eitsupi@users.noreply.github.com>
fix(create-docusaurus): fix readme docusaurus 2 ref (#9487)
fix(theme): fix firefox CSS :has() support bug (#9530)
fix(theme): docs html sidebar items should always be visible (#9531)
fix: v3 admonitions should support v2 title syntax for nested admonitions (#9535)
fix(theme-classic): fixed wrong cursor on dropdown menu in navbar, when window is small (#9398)
fix(theme): upgrade prism-react-renderer, fix html script and style tag highlighting (#9567)
fix: add v2 retrocompatible support for quoted admonitions (#9570)
fix(i18n): complete translations for theme-common.json Brazilian Portuguese (pt-BR) (#9477)
fix(content-blog): add baseUrl for author.image_url (#9581)
fix(type-aliases): add `title` prop for imported inline SVG React components (#9612)
fix(utils): Markdown link replacement with <> but no spaces (#9617)
fix(live-codeblock): stabilize react-live transformCode callback, fix editor/preview desync (#9631)
fix(cli): output help when no conventional config + no subcommand (#9648)
fix CI job (#9604)
fix Lint Autofix workflow (#9632)
fix(pwa-plugin): upgrade workbox (#9668)
fix(create-docusaurus): fix init template code blocks, and little improvements (#9696)
fix(theme): allow empty code blocks and live playgrounds (#9704)
fix(core): various broken anchor link fixes (#9732)
fix: remove old useless mdx typedefs (#9733)
fix(theme-common): fix missing code block MagicComments style in Visual Basic (.NET) 16 (#9727)
fix(core): conditionally include `hostname` parameter when using… (#9407)
fix(create-docusaurus): fix typo in init template sample docs (#9783)
fix(mdx-loader): allow spaces before `mdx-code-block` info string (#9776)
fix(core): links with target "_blank" should no be checked by the broken link checker (#9788)
fix(core): broken links optimization behaves differently than non-optimized logic (#9791)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: maintenance This PR does not produce any behavior differences to end users when upgrading.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants