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

Testing #23 #24

Closed
wants to merge 11 commits into from
Closed

Testing #23 #24

wants to merge 11 commits into from

Conversation

OnkarRuikar
Copy link

@OnkarRuikar OnkarRuikar commented Aug 2, 2023

Here we've tested:

# Because reviewdog submits suggestion if the
# OWNER ID of the PR's HEAD and the repository are the same, or
# the branch is created from the same repository.
# As a workaround, replace those OWNER IDs with `1`.
EVENT=$(jq '.pull_request.head.repo.owner.id = 1 | .pull_request.base.repo.owner.id = 1' diff/event.json)
echo "${EVENT}" > diff/event.json

@@ -7,17 +7,21 @@

游戏是最流行的计算机活动之一。新技术不断涌现,使得开发者有可能开发出更好、更强大、可以在任何符合标准的 web 浏览器运行的游戏。

## 开发网页游戏
## 开发网页游戏
Copy link

Choose a reason for hiding this comment

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

[mdn-linter] reported by reviewdog 🐶

Suggested change
## 开发网页游戏
## 开发网页游戏


欢迎来到 MDN 游戏开发中心!在网站的这一模块,我们为想要开发游戏的 web 开发者提供了资源。你可以在左侧主菜单中找到很多有用的教程和技术文档,尽情去探索吧。

我们同样提供了参考部分,因此你可以轻易地找到有关游戏开发所有常用的 API。

```css
div {background-color: red;}
Copy link

Choose a reason for hiding this comment

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

[mdn-linter] reported by reviewdog 🐶

Suggested change
div {background-color: red;}
div {
background-color: red;
}

files/zh-cn/games/index.md Outdated Show resolved Hide resolved
Comment on lines +28 to +29
* [关于 Emscripten](https://emscripten.org/docs/introducing_emscripten/about_emscripten.html)——简介和高级特性。
+ [下载和安装](https://emscripten.org/docs/getting_started/downloads.html)——安装工具链。
Copy link

Choose a reason for hiding this comment

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

[mdn-linter] reported by reviewdog 🐶

Suggested change
* [关于 Emscripten](https://emscripten.org/docs/introducing_emscripten/about_emscripten.html)——简介和高级特性。
+ [下载和安装](https://emscripten.org/docs/getting_started/downloads.html)——安装工具链。
- [关于 Emscripten](https://emscripten.org/docs/introducing_emscripten/about_emscripten.html)——简介和高级特性。
- [下载和安装](https://emscripten.org/docs/getting_started/downloads.html)——安装工具链。

Comment on lines +31 to +32
* abc
+ efg
Copy link

Choose a reason for hiding this comment

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

[mdn-linter] reported by reviewdog 🐶

Suggested change
* abc
+ efg
- abc
- efg

@OnkarRuikar
Copy link
Author

@yin1999 🎉 This is a clean solution because we get to keep the original reviewdog.

Could you implement this solution in mdn/content repo?

  • Update contents of pr-check_markdownlint.yml with pr-review-lint.yml, because we are doing only linting in the first part.
  • Rename second part to pr-lint-review-companion.yml.

@yin1999
Copy link
Owner

yin1999 commented Aug 2, 2023

@yin1999 🎉 This is a clean solution because we get to keep the original reviewdog.

Could you implement this solution in mdn/content repo?

  • Update contents of pr-check_markdownlint.yml with pr-review-lint.yml, because we are doing only linting in the first part.
  • Rename second part to pr-lint-review-companion.yml.

Of course I could. One more thing, should we recommend users to submit all suggested changes at once in PR review. I'm not sure will the reviewdog create duplicate suggestions?

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@OnkarRuikar
Copy link
Author

OnkarRuikar commented Aug 2, 2023

I'm not sure will the reviewdog create duplicate suggestions?

It doesn't seem to. I've later pushed a new commit containing only invalid list markers. Revewdog only posted a comment related to that.
Also later I merged only one suggestion and the workflow didn't upload any artifact to for the second part. Could you remove the front-matter linter part from the first workflow, it is throwing errors? Also, now instead of dumping env log the diff file for debugging cat ${TMP_DIR}/diff.

should we recommend users to submit all suggested changes at once

We can recommend them to do it in a single batch from the files tab as a good practice. Because committing one by one triggers multiple workflows every time, don't know if GitHub cancels prior running workflows before starting workflows for new commits.

We can make this suggestion in PR review companion's message explaining what the auto review is doing and how to accept these changes.

@yin1999
Copy link
Owner

yin1999 commented Aug 2, 2023

Thanks for the try and suggestions @OnkarRuikar 👍

@OnkarRuikar
Copy link
Author

@yin1999 could you hold off on the PR in mdn/content repo? Another approach has been suggested by Leo let me try that first.

@yin1999
Copy link
Owner

yin1999 commented Aug 2, 2023

@yin1999 could you hold off on the PR in mdn/content repo? Another approach has been suggested by Leo let me try that first.

Yes, please go ahead!

@yin1999
Copy link
Owner

yin1999 commented Aug 8, 2023

Closing as the new approach (mdn/content#28363) works great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants