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

Add autocorrect-node and configure that run lint before git commit. #14773

Merged
merged 17 commits into from
Dec 21, 2023

Conversation

huacnlee
Copy link
Contributor

@huacnlee huacnlee commented Jul 31, 2023

image

This will just be checking the changed files before git commit.

The autocorrect command provided by autocorrect-node NPM, there are no other binary need to install.

It also will follow the .autocorrectignore and .autocorrectrc rules.

Ref: #4191

@yin1999 @queengooborg

@huacnlee huacnlee requested a review from a team as a code owner July 31, 2023 13:43
@github-actions github-actions bot added the system Infrastructure and configuration for the project label Jul 31, 2023
@yin1999
Copy link
Member

yin1999 commented Jul 31, 2023

Hi @huacnlee. Should we also use autocorrect-node in autocorrect-lint workflow. As we could make sure the binary version is always the same.

By the way, do you have time to run a full autocorrect on zh-cn files again, and create some PRs to fix them :)

After that we should also add ci test to check if all the zh-cn documents are still "good" when we update the version of autocorrect-node.

@huacnlee huacnlee requested a review from a team as a code owner August 1, 2023 01:38
@huacnlee huacnlee requested review from yin1999 and removed request for a team August 1, 2023 01:38
@github-actions github-actions bot added the l10n-zh Issues related to Chinese content. label Aug 1, 2023
@huacnlee huacnlee force-pushed the feat/add-autocorrect-node branch 3 times, most recently from 31572b1 to e201d58 Compare August 1, 2023 01:57
@huacnlee
Copy link
Contributor Author

huacnlee commented Aug 1, 2023

image

Fail example

@huacnlee huacnlee force-pushed the feat/add-autocorrect-node branch 7 times, most recently from 5b511ef to df59f13 Compare August 1, 2023 02:48
@github-actions github-actions bot removed the l10n-zh Issues related to Chinese content. label Aug 1, 2023
@github-actions github-actions bot added the l10n-zh Issues related to Chinese content. label Aug 2, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2023

Preview URLs

(comment last updated: 2023-12-09 05:44:58)

@huacnlee huacnlee force-pushed the feat/add-autocorrect-node branch 6 times, most recently from 739532e to 1a1ea1d Compare August 2, 2023 02:40
@yin1999
Copy link
Member

yin1999 commented Sep 4, 2023

As there are security limitation on GitHub Actions. Could you merge the workflow here into pr-check-lint_content, or do the same changes as what we made in this workflow - spreading into two jobs and use pull_request_target event instead.

@github-actions github-actions bot added the merge conflicts 🚧 This pull request has merge conflicts that must be resolved. label Oct 10, 2023
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot removed the merge conflicts 🚧 This pull request has merge conflicts that must be resolved. label Oct 13, 2023
Copy link
Member

@yin1999 yin1999 left a comment

Choose a reason for hiding this comment

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

@huacnlee I've pushed some commits to merge autocorrect into our system. Could you have a look on this.

And I'm not sure about the ignore rules:

.autocorrectignore Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@huacnlee
Copy link
Contributor Author

@yin1999 The .autocorrectignore file is use same rules like the .gitignore.

Copy link
Member

@yin1999 yin1999 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @huacnlee.

ping @mdn/core-dev for the review from the development team

@github-actions github-actions bot added the merge conflicts 🚧 This pull request has merge conflicts that must be resolved. label Oct 30, 2023
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot removed the merge conflicts 🚧 This pull request has merge conflicts that must be resolved. label Oct 30, 2023
@github-actions github-actions bot added the merge conflicts 🚧 This pull request has merge conflicts that must be resolved. label Dec 7, 2023
Copy link
Contributor

github-actions bot commented Dec 7, 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 🚧 This pull request has merge conflicts that must be resolved. label Dec 9, 2023
@@ -61,6 +61,7 @@ jobs:
run: |
yarn markdownlint-cli2 --fix "**/${{ matrix.lang }}/**/*.md"
node ./scripts/check-url-locale.js --fix "files/${{ matrix.lang }}"
ls -d 2>/dev/null "files/${{ matrix.lang }}" "docs/${{ matrix.lang }}" | xargs yarn autocorrect --fix
Copy link
Member

Choose a reason for hiding this comment

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

Using ls -d to filter out non-existed dirs

@yin1999
Copy link
Member

yin1999 commented Dec 13, 2023

@bsmth Sorry to bother you, this PR has been there for a long time. And most of the content changes come from myself, I think it would be better to have a double check from other people before we merge this. Do you have time to have a look on this :)

@bsmth
Copy link
Member

bsmth commented Dec 15, 2023

Sorry to bother you, this PR has been there for a long time. And most of the content changes come from myself, I think it would be better to have a double check from other people before we merge this. Do you have time to have a look on this :)

This doesn't bother me at all, please continue to ping me on PRs where I can help review or unblock - that's what I'm here for :) I will have a look today 🙌🏻

# - correct: "Welcome, to read MDN Web Docs."
#
# More details:
# https://github.com/huacnlee/autocorrect
Copy link
Member

Choose a reason for hiding this comment

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

It might be nice to have this documented somewhere in this repo, but not a blocker for this PR.

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.

No objections from me apart from adding some docs / linking to https://github.com/huacnlee/autocorrect somewhere. A README.md in https://github.com/mdn/translated-content/tree/main/.github/workflows? We can do in a follow-up.

@bsmth
Copy link
Member

bsmth commented Dec 15, 2023

ping @mdn/core-dev for the review from the development team

It would be great to have a sign off from dev on this, also 👍🏻

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, only affects zh-CN.

@caugner caugner merged commit 3c87751 into mdn:main Dec 21, 2023
10 of 11 checks passed
mfuji09 pushed a commit to mfuji09/MDN-translated-content that referenced this pull request Dec 24, 2023
Improves "copywriting, correct spaces, words, and punctuations".
(Source: https://github.com/huacnlee/autocorrect)

Only enabled for zh-CN for now. Also runs before git commits.

Co-authored-by: A1lo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
l10n-zh Issues related to Chinese content. system Infrastructure and configuration for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants