MDN Web Docs
Automated Prettier formatting on pull requests #428
-
|
Hi all, I had promised to open a discussion about this topic, but I have only gotten around to it today, so my apologies for the delay. After significant efforts to get all of our content Prettified (thanks to all who helped), we had some trouble with pull request checks, diagnosing and solving issues, particularly those opened by contributors via the GitHub UI. It's clear we need some additional tooling that will help us with those cases. In the meantime, we merged a pull request to disable Prettier checks in pull requests: This is temporary while we get something in place to enforce the formatting requirements on incoming content without causing too much work for reviewers and authors. In the current state it's easier to automatically fix issues in content rather than troubleshoot broken CI. This is already handled in: https://github.com/mdn/content/blob/3e2369d97e2d6fbfe33a3c496a8edd90e0b539e2/package.json#L17 We have a great suggestion from @queengooborg to try out a GitHub action. This looks promising, but we will have to try it out. Another option is a GitHub bot that reacts to |
Beta Was this translation helpful? Give feedback.
Replies: 6 comments 12 replies
-
|
Another way is provide code suggestions, I just found a project named reviewdog. It has provided a tool that just used for Github Action. An example here: - name: Format changed content
if: ${{ env.GIT_DIFF_CONTENT }}
run: |
yarn prettier --write ${{ env.GIT_DIFF_CONTENT }}
- name: Submit suggestion
uses: reviewdog/action-suggester@v1
with:
tool_name: prettierBut this tool only handle |
Beta Was this translation helpful? Give feedback.
-
|
Part of my problem previously was that the precommit hook is buggy - at least on Windows. It was running but not fixing errors. FYI the output below. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for sharing, do we track this in an issue yet? |
Beta Was this translation helpful? Give feedback.
-
|
I've setup a fresh D:\github\content>git commit -am "test normal"
[STARTED] Preparing lint-staged...
[SUCCESS] Preparing lint-staged...
[STARTED] Running tasks for staged files...
[STARTED] .lintstagedrc.json — 1 file
[STARTED] * — 1 file
[STARTED] *.md — 1 file
[STARTED] tests/**/*.* — 0 files
[STARTED] *.{svg,png,jpeg,jpg,gif} — 0 files
[SKIPPED] tests/**/*.* — no files
[SKIPPED] *.{svg,png,jpeg,jpg,gif} — no files
[STARTED] prettier --ignore-unknown --write
[STARTED] markdownlint-cli2-fix
[SUCCESS] prettier --ignore-unknown --write
[SUCCESS] * — 1 file
[SUCCESS] markdownlint-cli2-fix
[STARTED] node scripts/front-matter_linter.js --fix true
[SUCCESS] node scripts/front-matter_linter.js --fix true
[SUCCESS] *.md — 1 file
[SUCCESS] .lintstagedrc.json — 1 file
[SUCCESS] Running tasks for staged files...
[STARTED] Applying modifications from tasks...
[SUCCESS] Applying modifications from tasks...
[STARTED] Cleaning up temporary files...
[SUCCESS] Cleaning up temporary files...
[test bfc513be9c] test normal
1 file changed, 1 insertion(+), 1 deletion(-)The log you've provided shows that you tried to make an empty commit. This may happen because the linters reverted your changes and there is nothing left to commit. --let a = 10;
++let a = 10; Reverting the changes done by the linters on error is a feature of |
Beta Was this translation helpful? Give feedback.
-
|
@OnkarRuikar SO what I did was modify some text in a js block so that the syntax is still valid, but not prettier compliant (I tested this by running prettier in VS code and seeing that the code was fixed). So with the non-prettier-compliant code I made the commit and got the empty commit. Why would the linter revert such a fix? I.e. I'd rather work out what is causing the hook to fail because I am concerned about unintended consequences of preserving the linter fixes on precommit hook fails. I don't know enough to evaluate. |
Beta Was this translation helpful? Give feedback.
-
|
@hamishwillee please share detailed steps to reproduce. Only way I could reproduce the exact error was to add double space at the end of any line and attempt a commit. |
Beta Was this translation helpful? Give feedback.
-
I think we should do this, and fix the precommit hook.
This fixes the main problem we had previously, which was leaving github only users in a state where they had a problem that they could not fix. It does it in a way that doesn't add any extra workflow for reviewers. What's the down side? |
Beta Was this translation helpful? Give feedback.
-
|
The command |
Beta Was this translation helpful? Give feedback.
-
|
I recall conversation about adding ESLint, but if I recall correctly, there are other challenges to overcome with ESLint as well. Often times in tutorials, we include code fragments as a part of walking the reader through the process, which aren't runnable on their own. We also include examples of code that is broken, or only works in a developer console. |
Beta Was this translation helpful? Give feedback.
-
|
Because it's a pain to see a new PR created every day. The size of the repo is already intractable and the fewer issue serial numbers and commits we add upon it the better. |
Beta Was this translation helpful? Give feedback.
-
Also to reduce burden on CI workflows and local dev environments, can we truncate the repo's history(commits etc.) to the last year and move all the older history to an archived repo? |
Beta Was this translation helpful? Give feedback.
-
|
The CI workflows aren't actually burdened by the number of commits we have. By default, the I don't think we're at a stage where we would need to consider truncating the commits yet. |
Beta Was this translation helpful? Give feedback.
-
|
That's right—I find the commit history useful for blaming, but at the same time it's about time to think about limiting activities to a reasonable amount. |
Beta Was this translation helpful? Give feedback.
-
|
We have now introduced a review bot that will suggest changes to help the contributor match Prettier's formatting desires. This should eliminate issues with Prettier formatting for changes made by contributors using the GitHub Web UI. If you have feedback on the bot, please feel free to reply to this thread, we'd love to hear what you think! |
Beta Was this translation helpful? Give feedback.
-
|
Hey all, @bsmth pointed me in the direction of this discussion after asking about the feasibility of hosting some kind of auto-linter bot outside of GitHub, and being me I couldn't just answer simply, I had to probe a bit into the motivations :) Whatever the solution is here unfortunately isn't simple, as GitHub's security model really locks down permissions in PRs coming from forks, for good reason: untrusted code run with the same permissions as code in a repo could do bad things, and if we work around it in any way, we need to ensure that whatever we're doing doesn't compromise that security model. Indeed, reviewdog doesn't appear to work in the way we'd like in those cases: https://github.com/reviewdog/reviewdog#graceful-degradation-for-pull-requests-from-forked-repositories But, ultimately, what's the motivation here? To fix linting errors for users who submit PRs exclusively from the GitHub UI? What proportion of content PRs come in like that? And since they all need to be reviewed anyway, why can't the reviewer just quickly run |
Beta Was this translation helpful? Give feedback.
-
|
This confuses contributors who use GitHub UI, if the reviewer doesn't have enough time to resolve it quickly. For me, if I find a minor error in the document (for example, a typo), I don't specifically open my editor, create a new branch, and then commit. I'd most likely click "Edit the page on GitHub." at the bottom of the page to fix minor error. But prettier may not like the changes, and it doesn't directly give me the suggestion about how to fix it, I will have to open my editor again, checkout the branch, and run
I think we can infer from the traffic statistics (as I'm just a collaborator on translated-content, I could only see the statistics on this repo), many people came from
And many new contributors just open PRs with the branch named like |
Beta Was this translation helpful? Give feedback.
We have now introduced a review bot that will suggest changes to help the contributor match Prettier's formatting desires. This should eliminate issues with Prettier formatting for changes made by contributors using the GitHub Web UI. If you have feedback on the bot, please feel free to reply to this thread, we'd love to hear what you think!