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

ci: add autocorrect-lint workflow for zh-CN #4191

Merged
merged 13 commits into from
Jul 26, 2023

Conversation

huacnlee
Copy link
Contributor

Continue for #3490, revert AutoCorrect again, improve CI for only check changed files.

@huacnlee huacnlee requested a review from a team February 18, 2022 07:13
@huacnlee huacnlee requested a review from a team as a code owner February 18, 2022 07:13
@huacnlee huacnlee requested review from schalkneethling and t7yang and removed request for a team February 18, 2022 07:13
@github-actions github-actions bot added l10n-zh Issues related to Chinese content. system Infrastructure and configuration for the project labels Feb 18, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 18, 2022

Preview URLs

Flaws (14)

URL: /zh-CN/docs/Web/JavaScript/Guide/Modules
Title: JavaScript 模块
Flaw count: 2

  • broken_links:
    • Anchor not lowercase
    • Is currently http:// but can become https://

URL: /zh-CN/docs/Web/JavaScript/Guide/Regular_Expressions
Title: 正则表达式
Flaw count: 12

  • broken_links:
    • Can't resolve /zh-CN/docs/JavaScript/Reference/Global_Objects/RegExp
    • Can't resolve /zh-CN/docs/Web/JavaScript/Guide/Regular_Expressions/Groups_and_Ranges
    • Can't resolve /zh-CN/docs/JavaScript/Reference/Global_Objects/RegExp
    • Can't resolve /zh-CN/docs/JavaScript/Reference/Global_Objects/RegExp/exec
    • Can use the English (en-US) link as a fallback
    • and 6 more flaws omitted
  • translation_differences:
    • Differences in the important macros (1 in common of 2 possible)

(comment last updated: 2023-01-17 14:24:31)

@irvin irvin requested review from irvin and SphinxKnight and removed request for t7yang February 21, 2022 07:46
files/zh-cn/learn/index.md Outdated Show resolved Hide resolved
@irvin
Copy link
Collaborator

irvin commented Feb 21, 2022

@huacnlee if i understand #3490 (comment) correctly, we should limit the lint to run only on the changed file and only if they are under /zh-cn folder, did the patch include these restrictions?

@huacnlee
Copy link
Contributor Author

huacnlee commented Feb 21, 2022

@huacnlee if i understand #3490 (comment) correctly, we should limit the lint to run only on the changed file and only if they are under /zh-cn folder, did the patch include these restrictions?

I think zh-tw and jp, kr need to discussion, AutoCorrect can support them, but the MDN Core team needs to consider to enable it.

@github-actions github-actions bot removed the l10n-zh Issues related to Chinese content. label Feb 21, 2022
@SphinxKnight SphinxKnight self-requested a review February 21, 2022 08:06
steps:
- uses: actions/checkout@v2
with:
fetch-depth: "30"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This means fetch recent 30 commits for get changed files.
In usually, a PR request commits times low that this number.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @huacnlee, we just find a better solution to get changes files, could you please reflect the changes made in #10390. If you need any help from me, feel free to @ me.

env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
BASE_SHA: ${{ github.event.pull_request.base.sha }}
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
run: |
# Use the GitHub API to get the list of changed files
# documenation: https://docs.github.com/rest/commits/commits#compare-two-commits
DIFF_DOCUMENTS=$(gh api repos/{owner}/{repo}/compare/${{ env.BASE_SHA }}...${{ env.HEAD_SHA }} \
--paginate \
--jq '.files | .[] | select(.status|IN("added", "modified", "renamed", "copied", "changed")) | .filename')
# filter out files that are not markdown
DIFF_DOCUMENTS=$(echo "${DIFF_DOCUMENTS}" | egrep -i "^files/.*\.md$" | xargs)
echo "DIFF_DOCUMENTS=${DIFF_DOCUMENTS}" >> $GITHUB_ENV

See also:

@huacnlee huacnlee force-pushed the feat/autocorrect-lint-again branch from c17e7cb to 057ebf2 Compare May 18, 2022 10:17
@huacnlee
Copy link
Contributor Author

huacnlee commented May 18, 2022

Hi @irvin @SphinxKnight, this Pull Request still waiting review to merge.

I just updated AutoCorrect to newest version.

For now, the CI script for detect changed files for linting looks works correct. All things done.

@github-actions github-actions bot removed the l10n-zh Issues related to Chinese content. label Jan 17, 2023
@yin1999
Copy link
Member

yin1999 commented Jan 18, 2023

Hi @bsmth would you like to have a look on this?

@bsmth
Copy link
Member

bsmth commented Feb 3, 2023

Hi @bsmth would you like to have a look on this?

Hi @yin1999, sorry for the late reply. Taking a look 👀

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.

I don't have objections to adding this so I'm leaving a +1. Maybe @caugner would like to have a look? 👀

See my comment about sharing the duplication in the shared workflows repo for future improvement. 👍🏻

@SphinxKnight
Copy link
Member

@bsmth
Copy link
Member

bsmth commented May 3, 2023

pinging @LeoMcA for another set of eyes 👀

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, but let's add some more comments to provide context.

.autocorrectignore Outdated Show resolved Hide resolved
.autocorrectignore Show resolved Hide resolved
.github/workflows/autocorrect-lint.yml Outdated Show resolved Hide resolved
@caugner caugner changed the title Revert to enable AutoCorrect again, and lint by git changed contents ci: add autocorrect-lint workflow for zh-CN May 29, 2023
@yin1999
Copy link
Member

yin1999 commented Jul 19, 2023

Hi @huacnlee, do you have time to take a look :)

Adding newline suggestion

Co-authored-by: Claas Augner <[email protected]>
@huacnlee
Copy link
Contributor Author

What can I need to do? Add the doc link?

@yin1999 yin1999 requested a review from caugner July 21, 2023 02:15
@huacnlee huacnlee requested a review from bsmth July 24, 2023 01:39
@yin1999 yin1999 requested a review from caugner July 24, 2023 01:40
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.

Thanks a lot, I see all of @caugner's comments are addressed and I think we should merge.

Thanks everyone 👍🏻

@bsmth bsmth merged commit 2dac809 into mdn:main Jul 26, 2023
5 of 6 checks passed
@queengooborg
Copy link
Collaborator

queengooborg commented Jul 27, 2023

I'm coming here after seeing CI failures in a few PRs I recently submitted to Prettier-ify the Simplified Chinese locale. I think that this was merged a little prematurely.

While the CI test is great, there's no easy way to apply the changes it is requesting. We have not added the package as a dependency, so we don't have easy access to the commands that allow us to locally test and fix reported issues. I should not have to rely on GitHub Actions to report issues in my commits; I should be able to run local, preconfigured commands to find all issues.

I will submit a follow-up PR to add the local commands and integrate this into our local workflows, once I've worked out the issues. In the meantime, I've disabled the workflow, and will re-enable it once we have set up a locally runnable command.

@yin1999
Copy link
Member

yin1999 commented Jul 28, 2023

I'm coming here after seeing CI failures in a few PRs I recently submitted to Prettier-ify the Simplified Chinese locale. I think that this was merged a little prematurely.

While the CI test is great, there's no easy way to apply the changes it is requesting. We have not added the package as a dependency, so we don't have easy access to the commands that allow us to locally test and fix reported issues. I should not have to rely on GitHub Actions to report issues in my commits; I should be able to run local, preconfigured commands to find all issues.

I will submit a follow-up PR to add the local commands and integrate this into our local workflows, once I've worked out the issues. In the meantime, I've disabled the workflow, and will re-enable it once we have set up a locally runnable command.

Yes. It's a better idea to add pre-commit hook to correct the format before committing. And we may need to fix the format of all zh-CN documents again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
system Infrastructure and configuration for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants