chore(CI): disable prettier check on pull requests

[bsmth]

Disabling the Prettier check on pull requests due to some speedbumps in reviewing

Motivation

We have some contributors hitting this which is causing some friction, particularly on PRs opened from GitHub's UI. There is no option to downgrade the errors to warnings that I'm aware of.

Additional details

We may want to instead already have this scheduled auto fix PR run Prettier instead:

https://github.com/mdn/content/blob/main/.github/workflows/markdown-lint-fix.yml#L30

i.e.:

      - name: Lint markdown files
        run: |
          yarn fix:md
          yarn fix:fm

https://github.com/mdn/content/blob/main/package.json#L17

Related issues and pull requests

• PR that introduced this check Add Prettier to lint command #20674
• Update index.md #26888
• WebAssembly | Memory - add note about allocation failure #27017

[Josh-Cena]

Why do we want to disable this? There are easy ways to format files without checking it out locally (namely, doing it from the Prettier playground), and the friction to do so is low compared to accepting one extra PR per day from the bot. We also already have errors of the same kind from markdownlint checks anyway.

[yarusome]

I have no objection to running Prettier checks, but the problem I've been facing with this new check is that it produces errors with absolutely no helpful info.

Although as Josh-Cena pointed out, contributors can rely on Prettier's playground for a zero-local-setup formatting action, but my fundamental hope is that there's a clear (but maybe a bit lengthy) guide of Prettier's style so that contributors can follow beforehand instead of afterward. However, there seems to be none in Prettier's doc.

[Josh-Cena]

It would be hard to codify it in prose, especially around indentation and line breaks. For example, there are obviously many ways you could format the following:

function getBareSection(section: Section): Section {
  assert(
    section.children.every(
      (s) =>
        /^[A-Z][A-Za-z]+\s*\(|^`|Record$/u.test(s.title) &&
        s.children.length === 0,
    ) ||
      section.children.filter((s) => /^get |^set /.test(s.title)).length === 2,
    `Not all children are AOs/type-defs for ${section.title}`,
  );
  return section;
}

and we don't want to say "you have to do this". The fact is that Prettier is constantly changing their heuristic to be "more idiomatic" as well.

[wbamberg]

Why do we want to disable this? There are easy ways to format files without checking it out locally (namely, doing it from the Prettier playground)

I don't think this is an acceptable workaround. Maintainers already have a lot to do and I'm not OK with adding more busywork into their day. It would be OK if we had a bot that ran Prettier when we made a comment like "/format", but in the absence of that I'd much prefer to have separate cleanup PRs.

[Josh-Cena]

We could add a note in the contributing guide about how contributors can do it themselves, but we should avoid at all costs extra PRs created every day. This repo is already growing at an uncontrollable rate.

[caugner]

It would be OK if we had a bot that ran Prettier when we made a comment like "/format"

Would anybody be interested in implementing this? ChatGPT gives some helpful instructions that sound plausible to me: https://chat.openai.com/share/67f598d7-89c2-491a-9a7b-a027120e6acf

(If it works, we could deploy it on MDN infrastructure and commit as @mdn-bot.)

[yin1999]

It would be OK if we had a bot that ran Prettier when we made a comment like "/format"

Would anybody be interested in implementing this? ChatGPT gives some helpful instructions that sound plausible to me: https://chat.openai.com/share/67f598d7-89c2-491a-9a7b-a027120e6acf

(If it works, we could deploy it on MDN infrastructure and commit as @mdn-bot.)

Hi @caugner, I'd like to take this. Could I have a try on it :)

[bsmth]

we should avoid at all costs extra PRs created every day.

I agree, just to clarify, the existing auto-fix workflow that's on a cron schedule already runs Prettier:

• https://github.com/mdn/content/blob/main/.github/workflows/markdown-lint-fix.yml#L27-L30

• content/package.json

  Line 17 in 3e2369d

  "fix:md": "markdownlint-cli2-fix \"**/*.md\" && prettier -w \"**/*.md\"",

I think Will is suggesting we can comment on an open PR with something like the following and it will check out the branch, format and commit the changes to "fix an open PR with broken formatting":

@mdn-bot /format

@mdn-bot prettier

@mdn-bot run prettier

[Josh-Cena]

That will be brilliant! Then we can even make the lint workflow required for merging to prevent first-time contributors from bypassing the check (because it requires approval).

[wbamberg]

Or why not just run Prettier in CI, as @hamishwillee said, if we are going to error when it hasn't been run.

[caugner]

Or why not just run Prettier in CI

Afaik we already run Prettier in CI, and I understood the issue is for contributors authoring directly on GitHub.

[wbamberg]

Or why not just run Prettier in CI

Afaik we already run Prettier in CI, and I understood the issue is for contributors authoring directly on GitHub.

Sorry if my terminology is incorrect. For GitHub UI contributions, we still run something, which generates preview pages and errors if Prettier has not been run. Instead of erroring, why doesn't it run Prettier? I suppose, because occasionally we don't want Prettier for some code blocks (because they are intended to show some specific formatting), and maybe the contributor didn't know about js-nolint, so we don't always want to run it.

[hamishwillee]

@caugner Yes ^^^. If we're always running a lint check and that will always fail unless prettier is run, then it is no reason not to auto-run prettier. It is possible (I guess) that prettier might fail, in which case we would still need instructions or better error information to help people resolve those cases.

[caugner]

For GitHub UI contributions, we still run something, which generates preview pages and errors if Prettier has not been run. Instead of erroring, why doesn't it run Prettier?

I think the reason you wouldn't want to run prettier proactively on PRs (and push the commit to the PR) is that it can cause the remote branch to diverge from the author's local branch.

Let's say I push the initial version of my contribution and open the PR, and then I make some more changes locally, but the prettier-fix on CI added a new commit on the remote, my next git push will fail, unless I do a git push --force or resolve the conflict by merging the remote or rebasing on the remote, which is often painful, especially for users with basic git knowledge.

On the other hand, because prettier is run as part of lint-staged, this case shouldn't arise, or the workflow could only run on PRs that include GitHub UI commits. 🤔

[wbamberg]

I think the reason you wouldn't want to run prettier proactively on PRs (and push the commit to the PR) is that it can cause the remote branch to diverge from the author's local branch.

But won't this also happen if people run /format (or paste the files into playground)? I'm failing to see the difference. Doesn't it also happen when I fix mdlint errors or typos by making and committing comments on the PR?

[jespertheend]

I'm not a frequent contributor and I ran into issues with the linter action.
I wouldn't mind linting being a requirement, but as a new contributor it is currently very difficult to figure out what needs to be done to make ci pass. The GitHub action log is not very helpful and CONTRIBUTING.md says nothing about linting.

By looking at the output of yarn run I made a guess that yarn run fix:md was the command I was looking for. But it hangs at linting: 11789 file(s) for a good minute, and linting the actual files takes a few minutes as well. Only to remove two spaces from the single file I edited :/

On top of the ones already mentioned, another solution could be https://github.com/reviewdog/action-suggester, which leaves suggestion comments which you can apply from the GitHub UI. The downside of this is that if you touched many files without formatting, you'll end up with tons of comments.

[OnkarRuikar]

Above ^^^ is a legit concern.
That is exactly why the daily cleanup bot was put in place. The idea was to have safety net in following cases:

• case of new contributions requiring approval
• too many lint issues to fix and reviewer don't want to force on the contributor or fix themselves
• when reviewer don't want to drag the PR further

We don't have to completely disable this. Reviewers could simply merge PRs with failing lint workflows, knowing it will be auto fixed midnight. Some reviewers may wish to fix small issues, like trailing spaces, themselves.

There are easy ways to format files without checking it out locally (namely, doing it from the Prettier playground)

When Prettier raises an error, we could give link to the playground in error message, in a PR comment, or permanently in PR companion's message.

[yarusome]

For a priori measures to reduce the frictions caused by Prettier, I think it's also helpful to:

1. List any simple rules enforced by Prettier in the writing style guide;
2. (optionally) also add a subset of the rules above which can be expressed as regexps in .markdownlint-cli2.jsonc.

I could have a try at Point 1 when I have time.

[Josh-Cena]

@yarusome The point of Prettier was to make people write code any way they want and have it automatically uniformized. If we have to ask them to write code in a particular way that pleases Prettier, then it kind of loses the point.

The only kind of fix that I find reasonable is to run the fix by a bot, preferably on the PR. Of course, educating contributors about how they may run Prettier for their contributions themselves sounds more efficient, but it seems other members here still find it too demanding.

[rubiesonthesky]

Could it work to mitigate accidental branch diverging, if bot would add a comment to PR that format is not correct and offer auto format? Something along lines “code formatting has issues. To auto fix issues, please comment ‘@mdn-bot format’”. This way it will be transparent that there was problem and the submitter can fix the issue before reviewer needs to do anything. Or reviewer could also accept the formatting changes if submitter is using GitHub UI.

[hamishwillee]

The fact that this is still not merged results in constant frustration. Just had a rough night, and right now the idiocy of this makes me furious.

It's a barrier to contribution and needs to be fixed sooner rather than "whenever". Disable it and work out what you want to do as an alternative afterwards.

FWIW IMO prettier should be run absolutely every time, both locally on commit AND on the server as part of the workflow.
There is a downside to this too - which is that you'll have to resync to upstream to get server-side changes if you did not run prettier locally (but if you run in both places that should not happen).

[wbamberg]

Yes, can we try to resolve this? I can see three options, any of which are OK:

1. disable Prettier check in CI - this is OK because we do auto catch-up PRs.
2. have a /format command that maintainers can run
3. auto-run Prettier in CI

I have a slight preference for (2) but would take any of these over what we have now.

[hamishwillee]

Yup.

auto-run Prettier in CI

I prefer this (3), but only if we also run prettier locally as part of commit (that avoids having different local and upstream changes, while still working for Github only changes).

[OnkarRuikar]

have a /format command that maintainers can run

+1 This fixing has to be voluntary to avoid diverging branches. Show a message in PR, when prettier fails, telling what happened and how to run the /format command in comment. After fixing done remove the message.
Only, the author and maintainers need to have permission to this command.

[hamishwillee]

+1 This fixing has to be voluntary to avoid diverging branches.

^^^ Fairly strongly disagree. If things can update independently on github and locally you're always likely to have to do a merge or rebase anyway - I always update first by habit.
The thing is, as a writer you get a clear warning at the point you upload that you're out of sync. That's easy to fix at the time you're doing your upload rather than a day later when you notice the error email from CI.

I'll say it again: if a users always has to "go local" and run [insert whatever tool you like] to fix a linter problem, then it should be run automatically both locally and on the server. That means most of the time you will not get a sync error unless someone makes changes on github which you have to sync anyway. That is the case for prettier, as it was for image upload, and so on.

[queengooborg]

3. auto-run Prettier in CI

If we want go for this option, I've just stumbled upon a workflow that I think will suit our needs: https://github.com/marketplace/actions/prettier-action

[bsmth]

The fact that this is still not merged results in constant frustration. Just had a rough night, and right now the idiocy of this makes me furious.

So sorry this is still causing trouble 😢

3. auto-run Prettier in CI
   If we want go for this option, I've just stumbled upon a workflow that I think will suit our needs: https://github.com/marketplace/actions/prettier-action

Thanks for the tip, @queengooborg shall we give it a shot in a test repo?

edit:

I have a slight preference for (2) but would take any of these over what we have now.

Same here, I put together a demo express app that can handle these types of commands but if we're going to enable it in CI, we probably don't need it.

[Rumyra]

We discussed this in our weekly content sync and for the moment we are going to disable - discussion to carry on discussing incoming

[bsmth]

Hey everyone, this one was merged already because we can stop the annoyances in CI right away while keeping the auto-fix job, but it would be great to re-enable it when we have tried the alternatives mentioned above - I started a discussion here if you'd like to chime in for next steps: https://github.com/orgs/mdn/discussions/428