Ideal set-up for Markdownlint

[teoli2003]

Hi all!

For a long time, we have wanted to have Markdownlint taking care of our documents, easing us from the cumbersome formatting.

Lately, we have activated the automatic test of your PRs. Markdownlint flags errors it detects. This is 🎉! I'm really happy about this.

Nevertheless, I'm not sure we have the same view on what the final state should be. I want to open this discussion so we can agree on what is left to call the Markdownlint project done.

This is what we want (in my understanding):

Activate Markdownlint as a test on each PR (PR: mdn/content#12316)

1. Only create Warnings (for automatically fixable problems). (Make lint annotations warnings content#19868)
2. Restrict checks to the modified PRs only. (Lint only modified files content#19884)
3. Add a command to correct fixable MD problems in a PR automatically. Something like @mdn-bot fixmd that the author or the reviewer can call.
4. Automatically run the script and create a commit when the merge button is pressed.
5. Create actual errors (=== we don't want to merge a PR with these) if:
   - MD045: an alt is missing on an image (As soon as the add missing alt project is over)
   - MD023: the position of a heading is not at the start of the line (this is problematic for React and Yari)

Run a daily Markdownlint script automatically to create a PR to fix MD errors (PR: mdn/content#16036)

1. Fix what can be fixed (chore: Add Markdownlint cleanup job content#16036)
2. Log unfixable errors in the PR description
3. Eventually, automatically merge this PR (after some time of testing)

I would like us to synchronize our views and agree on the work needed (where we are going with MDLint). I'll update the points above as the discussion progress.

cc/ @wbamberg @caugner @nschonni @OnkarRuikar @Josh-Cena (and everybody else, of course)

[bsmth]

Hi Jean-Yves,

I think the linting and formatting efforts going on right now are great, this is a really good initiative 🙌🏻

Your suggestions for activating Markdownlint as a test on each PR (1->5) and the daily Markdownlint script (1->3) also makes sense.

I've previously exclusively used Prettier for taking care of project formatting. I have a general question; what does Markdownlint do that Prettier cannot do and how do the two tools interact? There is a doc on MDLs repo that says both work together well, but I'm sure you can give more context!

[teoli2003]

I don't know yet. We are close to having Markdownlint set up, but I think we are still further away from Prettier. Also, this allows us to understand what is needed to automate such tools and will ease our work when automating Prettier. (We need to run Prettier on the whole of MDN, defining what we want to exclude, …)

Maybe we won't need Markdownlint anymore when Prettier is automated (but I think the points above will apply almost identically to Prettier).

(And Markdownlint is less "intrusive" in case something goes wrong.)

[bsmth]

OK, I understand, thanks for the background.

Maybe we won't need Markdownlint anymore when Prettier is automated

That's my gut feeling, but I'm curious how it turns out in practice. I'm looking forward to seeing it all come together, this will be a great help for maintenance.

[caugner]

IMO Prettier is more opinionated, therefore less configurable and has fewer specific Markdown rules, as it's more versatile.

[OnkarRuikar]

From the daily lint runs so far, the average number of files fixed by the linter per day is less than 4. So I think just the daily Markdownlint cron job will also work.

---

We can also drop the lint check and directly have auto fix that will trigger on each commit to the PR:

---

If I can manually trigger (like @mdn-bot mdlin) that's fine too, as the reviewer will do it if needed.

There is a possibility that this can be achieved:
Method A
We can trigger a workflow on any event related to the pull request. Refer events that can trigger a workflow

• So we write a workflow that will trigger on pull_request_comment event.
• The workflow will read all the comments and look for the command and will do the lint:

      - name: Find Comment
        uses: peter-evans/find-comment@v2
        id: fc
        with:
          body-includes: {{Perform_Markdown_Lint_fix}}
          direction: last
      - run: |
          echo ${{ steps.fc.outputs.comment-author }}
          # if comment author is PR creator or reviewer do the lint

With this we can drop a command in the PR comments like {{Perform_Markdown_Lint_fix}} or {{Perform_JavaScript_Lint_fix}}.

We can re-purpose other suitable pull request related events too. But comments will be the least complicated approach.

---

Method B
Create a separate workflow that has a manual trigger:

Here in input field we provide the PR number and run the workflow.
As Allow edits and access to secrets by maintainers is checked by default on the PRs, the workflow will be able to make commits to the PR's branch.

This won't happen on the PR page. You will have to do it from repo's actions page.
Or make a POST request using curl:

curl \
  -X POST \
  -H "Accept: application/vnd.github+json" \ 
  -H "Authorization: token <TOKEN>" \
  https://api.github.com/repos/mdn/content/actions/workflows/markdownlint_fix/dispatches \
  -d '{"ref":"main","inputs":{"PR":"193838"}}'

---

Need to do some experiments around this.
Guys, WDYT?

[teoli2003]

I agree, the question here, is how to trigger it. If it is triggered after each commit, when I locally try to push afterwards it will give me an error. (I need to force push, or to pull before modifying locally).

If I can manually trigger (like @mdn-bot mdlin) that's fine too, as the reviewer will do it if needed.

[Josh-Cena]

CI doesn't block push whatsoever? I'm not sure what's happening, but it's only meant for an indication.

[teoli2003]

I think there is a misunderstanding, we don't speak about CI blocking push. We are speaking about triggering an extra commit if there is a MDlint error detected, after another commit.

[Josh-Cena]

Ah sorry. Yes, having CI push to someone else's branch is a bad thing to do in general—a cron job is good enough.

[nschonni]

mdn/content#16036 also allows the job to be manually triggered if someone wants the cleanup to happen earlier

[wbamberg]

Sorry in advance this is going to be a bit of a ramble.

I want to think about when and how often and in which circumstances linter changes should just always be applied, and when they need manual intervention and checking, and how this should affect contributor and reviewer workflows.

IIUC I think we're looking at a combination of:

1. Prettier/markdownlint errors visible as warnings in the PR
2. A manual command a reviewer can run (like as a comment like /fix-lint) that fixes them (and therefore makes a commit in the contributor's branch).
3. Periodic automated PRs to fix any accumulated errors

Problems with (2) are:

• (as noted) it means contributors have to pull again if they want to make follow-up changes. But we do already sometimes push small corrections to contributor branches, so this isn't a totally new thing.
• it's extra friction for reviewers, who will very often:
  • have to run this command, and wait for all the CI checks, and the previews to be built, before they could do a final review (note also that many PRs to mdn/content are from first-time contributors, so we have to do this already once before any workflows run. So now we would have to do it twice).
  • have to check, before running the fix command, whether these are legit errors or whether they are the very rare cases where we should inhibit the check (for example because the code has to be preformatted).

One of the things that makes this hard to reason about is that it's almost fully automatable, but there are these rare cases when manual override is wanted. And also that manual checking for things like spacing and indentation is hard for humans (well it is hard for this human anyway). So we don't really want to pay the full cost of manually reviewing every formatting change, but we do need to catch the rare exceptions.

I think that's also why I quite like (3): it means we can concentrate on these questions in a dedicated PR, instead of it being another checkist item for every other PR review.

So as a reviewer I'd be interested to know whether running (2) is an expectation or not. I also think we can experiment: how does it feel to run (2)? If we don't run (2), how unwieldy does reviewing (3) get? (based on our experiences with @OnkarRuikar 's auto-fix PRs, it doesn't seem at all bad).

I think ESLint is a different matter and is much more likely to need intervention at (2).

[Josh-Cena]

I would prefer it to stay red. This signals maintainers to self-fix if possible before merging.

[Josh-Cena]

Reviewers—yes. Sometimes when I see a red cross, even when I'm not the assigned reviewer and I don't intend to review it, I would do a drive-by fix to make CI happy. I don't always catch the annotations in the diff either, if there's no red cross.

[OnkarRuikar]

@Josh-Cena if we add a label ‘lint errors 🧶’ then it’ll also solve your requirement. Even better, search by tag could be done to find all the PRs with lint errors.
And we mark commits red only when there are unfixable errors.

[nschonni]

If someone wants to create a label, it shouldn't be difficult to add on if: failure() task to add the label during the PR Markdownlint job

[wbamberg]

One of the things that makes this hard to reason about is that it's almost fully automatable, but there are these rare cases when manual override is wanted.

I can think of a few cases for Prettier, but I have yet to be convinced of one exception for Markdownlint. If there are some, it means we would have a 3rd case above:

What I mean is: "fully automatable" means we just fix the errors with no human intervention at all. But we're not doing this: we are having a daily PR, that's getting a manual review. This should be quick 99% of the time, but we're doing it (I guess) because 1% of the time there may be some weird exception we need to handle differently.

I think that's also why I quite like (3): it means we can concentrate on these questions in a dedicated PR, instead of it being another checklist item for every other PR review.

I'm less convinced by this. I don't think we want to start our Mondays by crafting the six alt texts that have been forgotten over the last week, meaning we have to dig into the subject of each of the affected and unrelated articles.

I agree about the case of alt, for sure. I think that should be an error blocking the PR from being merged. I guess once we get into ESLint there will be more of these too.

The current expectation is that the Markdownlint workflow being red can be ignored.

This expectation seems to be broken by the case of alt, doesn't it?

[Rumyra]

Related #211

[caugner]

Configuring lint-staged and husky would prevent everyone not editing via GitHub UI from committing any non-fixable errors, while automatically fixing fixable errors.

See:

• build(lint): use lint-staged to run prettier and eslint when comitting yari#5671
• Enforce formatting using Prettier and Husky+lint-staged bob#866

[teoli2003]

Yes! This is a good idea, it would complement well the two other workflows we have (detecting and warning about fixable errors and automatically fixing them every day).

I see that mdn/content#20397 has been filled to add them.

[teoli2003]

Note we likely would like to add husky+lint-staged with Prettier and ESLint to our examples directories (dom-examples, …) too. But we need to let @schalkneethling finish their consolidation first.

It would help us gather more experience with ESLint, too.

[bsmth]

Minor data point since this has landed:

I am using Prettier as my formatter in VSCode in a number of cases (on file save etc.). Since mdn/content#20397, I am commenting out the *.md line in the prettierignore so that I can run this locally before staging changes. I'm guessing this is a workaround for the moment until we have some integration with Prettier, but just an FYI.

[nschonni]

mdn/content#20674 will start to opt-in the already clean folders. You can also use one of those opt-in patterns if you just want some of the files to start to cleaned

[bsmth]

Ah okay, this is what they refer to in the docs as "negative patterns" (!not/this/path)? Thanks for the hint

[nschonni]

Yup, for mdn/content#20677 I uncommented out the mozilla line in the 20674 branch, then ran yarn prettier -w .

[rubiesonthesky]

Would it be possible / useful to use prettier's pragma option? So already formatted pages would be marked with pragma and if set with config, only those pages would be formatted with prettier. When formatting new pages you need to disable requiring pragma. This could help with lint staging and in PR stage to ensure that once fornatted pages would stay formatted but there would be no unexpected formatting.

https://prettier.io/docs/en/options.html#require-pragma

I don't have personal experience to weight how well this would help with mdn projects. But since this is especially made for situations where formatting is done in piece by piece, I think, it's worth looking into. (If that was not already done.)

[nschonni]

I was thinking in a smilar vein of "Markdown linting" rather than "Markdownlint" ideal setup, would be to have the yari flaws fixed by Husky on commit. I know some of them probably can't, and others might be too expensive to run on a large number of files in a commit.
When I was playing around though, I couldn't figure out a way to do it by the file path, rather than the slug, since only the file path would be available to lint-staged

[OnkarRuikar]

I couldn't figure out a way to do it by the file path, rather than the slug, since only the file path would be available to lint-staged

We could use sed to convert file paths to slugs: 🤔

$ echo 'files/en-us/web/accessibility/aria/forms/index.md'| sed 's|files/en-us/||; s|/index.md||' | xargs -I{} yarn yari-tool flaws {}
yarn run v1.22.19
/home/koder/GitHub/mdn/content/node_modules/.bin/yari-tool flaws web/accessibility/aria/forms

In above code we have to replace echo with staged file paths.

---

Whether auto fix or not, I think we shouldn't enforce this before fixing all the flaws in content. Because contributors will start seeing unrelated changes/fixes in their commits.