chore: make Markdownlint a dependency and script #12316

nschonni · 2022-01-21T19:43:20Z

Not expanding the globs or adding the config like the previous PR.

Add to lockfile to prevent failures as versions are release
Add script to control runs going forward
Dropped exceptions for CODE_OF_CONDUCT and LICSENSE since they don't seem to have issues right now with current config

nschonni · 2022-02-03T05:16:21Z

Need to call npm ci now because it's not using NPX now. Marking as draft, as it probably makes sense to land #12313 before and then fix this one up

nschonni · 2022-03-18T20:26:15Z

@hamishwillee I rebased for the conflicts, do you want to take a look at this?

hamishwillee · 2022-03-21T03:57:02Z

@nschonni Would love to. I was able to install it - though first it says I needed to install Node 14 (oddly when I used 12 it told me I needed >=14, but when I tried 17.4 it told me I needed < 17).

But I need a bit more info on how to test it - how is is supposed to work?

I can see you have add yarn link:md which seems to lint every single file. But when I run it locally nothing gets reported:

D:\github\mozilla\mdn_content\content>yarn lint:md
yarn run v1.22.5
$ markdownlint "*.md" "rfcs/**/*.md"
Done in 0.30s.

Further it looks like perhaps this is run on a workflow just on the pages you have modified.

Upshot, what do you expect this to do/how is it triggered? Then I can tell you if it does that :-)

nschonni · 2022-03-21T04:54:27Z

This still isn't running on all pages, it only runs on the root MD and RFC files like it does today. This just pins it to the latest version of Markdownlint and adds a problem matcher for any issues that the CI picks up.
Next step would be to expand out to all files in the repo, but last time I tried the bigger globbing, the PR stalled out, so this is taking smaller steps to get there.

nschonni

These would be the required changes to expand the linting to all files

.github/workflows/markdown-lint.yml

package.json

nschonni · 2022-03-22T01:50:15Z

@hamishwillee I added a separate commit expanding the globs, so it's easy to reverse.
If you look on the files tab now, you'll what the problem matcher now adds

I think last time the larger glob stalled out pending some discussion around documentation and/or integrating it in with the other flaw checks.

hamishwillee · 2022-03-22T02:03:25Z

@nschonni That's actually pretty cool.

The things that spring to mind:

List on files tab is "Unchanged files with check annotations". I would only be interested in the changed files in "real life" - I shouldn't have a report on things I didn't touch.
If I'm running this locally, how would I generate this changed files report? I.e. I generally want to fix problems before I push up to github and I only want to run on the things that be in the PR

nschonni · 2022-03-22T02:10:53Z

List on files tab is "Unchanged files with check annotations". I would only be interested in the changed files in "real life" - I shouldn't have a report on things I didn't touch.

Those will show up inline in the changed files as well, but at the start, there may be a few that show up the "Other files" as PRs land with markdownlint issues at the start

2. If I'm running this locally, how would I generate this changed files report? I.e. I generally want to fix problems before I push up to github and I only want to run on the things that be in the PR

You can get the same thing locally by running yarn lint:md which will show the same console output as https://github.com/mdn/content/runs/5637402653?check_suite_focus=true#step:5:7
The problem matcher is something that uses regex to transform console outputs to inline annotations in the PR view

hamishwillee · 2022-03-22T02:45:21Z

Then it sounds great to me. It's certainly a good start, and not much impact if people want to ignore it. i.e. I'd be happy to merge this. We should get a sanity check from someone else in the team though - probably Will Bamberg would be my first suggestion.

PS Though not completely clear on your response to "1". To be clear in my response, a report like this is only useful if I can still "see the wood for the trees". If there are too many files in a report that I did not touch then I won't bother looking at it.

nschonni · 2022-03-22T03:02:23Z

PS Though not completely clear on your response to "1". To be clear in my response, a report like this is only useful if I can still "see the wood for the trees". If there are too many files in a report that I did not touch then I won't bother looking at it.

The "Other files" section caps out at the first 10 as far as I know, and the files actually touched will have the inline annotations on the files in the regular file diff lines. People/reviewers can ignore and merge the PR if the only issues are in the "Other files" section, but the build will show as failing

nschonni · 2022-03-22T03:02:57Z

@wbamberg do you want to take a look too?

nschonni · 2022-03-22T19:31:15Z

package.json

@@ -11,11 +11,13 @@
    "start": "yarn up-to-date-check && env-cmd --silent cross-env CONTENT_ROOT=files REACT_APP_DISABLE_AUTH=true BUILD_OUT_ROOT=build node node_modules/@mdn/yari/server",
    "filecheck": "env-cmd --silent cross-env CONTENT_ROOT=files node node_modules/@mdn/yari/filecheck/cli.js --cwd=.",
    "content": "env-cmd --silent cross-env CONTENT_ROOT=files node node_modules/@mdn/yari/tool/cli.js",
-    "build": "env-cmd --silent cross-env CONTENT_ROOT=files BUILD_OUT_ROOT=build node node_modules/@mdn/yari/build/cli.js"
+    "build": "env-cmd --silent cross-env CONTENT_ROOT=files BUILD_OUT_ROOT=build node node_modules/@mdn/yari/build/cli.js",
+    "lint:md": "markdownlint \"**/*.md\" -i node_modules"


Would it be helpfull to have a "fix" target like fix:md to go with the lint? EX: "fix:md": "markdownlint \"**/*.md\" -i node_modules --fix"

Hmmm. I would definitely like to auto fix any cases of too many empty lines at the end. But without a better view of the full set of things it might fix, not sure I'd like to do this as a blanket thing.

I guess perhaps the answer should be "yes" and then we tune it until that is something we're happy to just do. Otherwise this can never be a CI mandated thing.

OK, I can add the extra target. Does it make sense to keep the scoped :md, or just have them as lint and fix

I don't think it matters all that much, but personally I'd keep md because there are some html docs still in-tree.

Is there any reason not to use lint instead of lint:md, and remove fix:md, as folks can just run npm run lint -- --fix themselves?

We know about the --fix, and it could be documented, but it's more user friendly to have the fix target for new users

Hm, I understand. Maybe still call it lint and lint-fix, since there aren't any other lint scripts for now.

@caugner There is going to be one for JavaScript.

Okay, makes sense then. Maybe linting JavaScript and Markdown could be integrated via ESLint (eslint-plugin-markdownlint + eslint-plugin-prettier) in the future.

nschonni · 2022-03-23T19:50:00Z

Added the fix, and opened #14231 to cleanup the existing issues

hamishwillee · 2022-03-24T23:24:06Z

Added the fix, and opened #14231 to cleanup the existing issues

Was that really all of them?

In any case, looking at that issue I'd say that the changes are all reasonable and could be automated by default.

nschonni · 2022-03-24T23:37:21Z

Yeah, I had cleaned up a bunch over the last few months. Some of the rules in Markdownlint can probably still be cleaned up like MD001, but can't be automatically fixed so I haven't dug into them much.
There is 1 minor spacing issue that landed, but if this lands, it should be straight forward to run yarn fix:md and submit another cleanup PR

wbamberg · 2022-03-25T02:41:28Z

Sorry to be slow getting to this. I will have a look tomorrow.

wbamberg · 2022-03-29T23:32:51Z

Thanks for this PR, @nschonni !

I would like to understand what the contributor experience is like with this PR. If someone makes a change that is not md-lint compatible, does it look like a CI failure? What do we as maintainers do with a PR that is not md-lint compatible? Is it easy for us to fix the problems ourselves?

I like linting, and think we should have it, but linting is a pretty contributor-hostile thing unless the tooling is helpful. A lot of our contributions are very much casual drive-bys, and most people aren't going to want to deal with tooling that makes contributions harder. So I think it is important to consider contributor experience, not just for markdownlint but for other tools like Prettier and ESLint.

nschonni · 2022-08-17T16:04:42Z

@caugner ended up rebasing from a new Yarn.lock conflict. I'll update #16036 after this lands, which should help cleanup any existing issues

OnkarRuikar · 2022-08-18T06:47:12Z

After this PR lands can we make one improvement? Process only the modified files in PRs.

Something like this: [ref]

- name: Lint markdown files
- id: files
  uses: jitterbit/get-changed-files@v1
  with:
    format: 'md'
- run: |
    for changed_file in ${{ steps.files.outputs.all }}; do
      markdownlint ${{ changed_file }}
    done

ATM this processes files which are not part of the PR as well. For example, https://github.com/mdn/content/pull/12316/files#diff-e917862ed5ced988f4072a8ccf20596eccf57cc889f0c897aef8d2c8016bfb72

simplescreenrecorder-2022-08-18_11.53.12.mp4

In above video this PR got unrelated files flagged. Which could be confusing to the contributors. And annoying for maintainers.

If very first commit in a day creates a lint error then every PR(~30 a day) will get these files flagged throughout the day till the daily markdownlint happens. Even after the daily cleanup happens those flagged files remain in the PRs forever, adding the unnecessary redundancy.
Also, this will put burden on reviewers to correct every lint error before the merge, because they'll get blamed for the lint errors showing in every next PRs. So it'll also defeat the purpose of the 'daily markdownlint' task.

It would be great if we could get this done sooner, but let's not stop this PR.

teoli2003 · 2022-08-18T06:58:01Z

So if I understand well, we need to fix these to be able to merge this:

teoli2003 · 2022-08-18T07:02:15Z

If the very first commit in a day creates a lint error, then every PR(~30 a day) will get these files flagged throughout the day till the daily markdownlint happens.

My understanding is that this very PR will prevent landing a PR that adds a Markdown error in the first place. So this won't happen.

But I agree we should test only the updated files (as the others are good anyway), just for performance reasons. A follow-up looks good to me.

OnkarRuikar · 2022-08-18T07:15:56Z

My understanding is that this very PR will prevent landing a PR that adds a Markdown error in the first place. So this won't happen.

Fixing lint errors shouldn't be mandatory for the contributors or reviewers. They should be allowed to ignore the lint errors for ease of contributing and speed. Also because we are going to have automated daily lint(with fix) job as a safety net.

Fixing trailing spaces and missing lines is easy but fixing indentations and lists with code blocks or multiple paragraphs are not easy. Especially definition lists with sub lists gave me hard time. Without using intelligent IDE/editor it's a nightmare.
Contributors may end up doing many trial and errors till the preview companion shows desired output. Each try will be slow as the companion takes time to deploy changes. 🤒

teoli2003 · 2022-08-18T07:43:24Z

We should configure this test as non-blocking merging. (And your Daily markdownlint PR should be created automatically by your bot, with its configuration in this repo so we can discuss and adapt its configuration through PR.

What I see is:

this action testing and warning in the PR
a bot (Daily Markdownlint) running daily and fixing automatically what can be fixed (after some time we may even auto-commit that daily PR as we do with dependabot and dependencies).

So the next steps are to fix these 4-5 errors, land this PR, and configure it to be non-mandatory for merging a PR.

OnkarRuikar · 2022-08-18T08:11:30Z

So the next steps are to fix these 4-5 errors,

It's been handled already in Daily markdownlint.
One more rebase is needed to get rid of these. Hopefully no new lint errors have been introduced 😄

land this PR, and configure it to be non-mandatory for merging a PR.

I think we can make this non-mandatory in this PR itself. By simply adding one line "severity": "warning" [ref]. By default it's an error.

teoli2003 · 2022-08-18T08:13:24Z

Let's do this! We can bump it again in a later phase if we want.

.github/workflows/markdownlint-problem-matcher.json

teoli2003 · 2022-08-18T08:19:07Z

So if I understand well, we need to fix these to be able to merge this:

By the way, these 4 errors are fixed on main, so a simple rebase would fix them, but they are fixed.

OnkarRuikar · 2022-08-18T08:37:13Z

.github/workflows/markdown-lint.yml

      - name: Lint markdown files
        run: |
-          npx markdownlint-cli '*.md' -i LICENSE.md -i CODE_OF_CONDUCT.md
+          echo "::add-matcher::.github/workflows/markdownlint-problem-matcher.json"
+          yarn lint:md


Suggested change

yarn lint:md

(yarn lint:md || exit 0)

To not exit the linter with error code 1.

On the other hand, as @nschonni has pointed out, the failing check does not prevent a PR from being merged once it is approved.

This is because the following option is disabled in the protection rules for the main branch:

nschonni · 2022-08-18T14:34:44Z

@teoli2003 I rebased, but there are now additional issues. Is the merged blocked by the failing job? My understanding is that because none of the jobs are marked as "Required Checks" for the branch rules, it should disable the merge buttton, but rather just make it not-green

teoli2003 · 2022-08-19T06:00:44Z

The red test is not critical: it will be fixed in the next Daily markdownlint PR.

The only thing preventing us from merging is to be sure that this is not marked as mandatory so that it can be merged. In a follow-up, we have to be sure it applies only in the files inside the PR because it won't help much (it will be almost forever red, with no way to get it green in the PR – this is happening here.).

OnkarRuikar · 2022-08-19T07:07:01Z

Guys, my suggested changes to make it non blocking work!
Refer: OnkarRuikar#1

Lint issues as warning annotations instead of errors:

Green `Merge pull request` button despite the warnings:

teoli2003 · 2022-08-19T07:10:20Z

Should we go with it, @nschonni, @caugner, @Elchi3? We can always tighten things more. (And we will have a bot soon to fix these warnings automatically when possible)

nschonni · 2022-08-19T15:14:53Z

I would say go with it, and if proves to be too disruptful after a few weeks, Actions can be disabled from the GitHub UI without actually deleting the file

Disable new failing rules

caugner

LGTM

PS: For me, this would be a style(md): instead of chore:.

caugner · 2022-08-22T10:23:05Z

package.json

@@ -11,11 +11,13 @@
    "start": "yarn up-to-date-check && env-cmd --silent cross-env CONTENT_ROOT=files REACT_APP_DISABLE_AUTH=true BUILD_OUT_ROOT=build node node_modules/@mdn/yari/server",
    "filecheck": "env-cmd --silent cross-env CONTENT_ROOT=files node node_modules/@mdn/yari/filecheck/cli.js --cwd=.",
    "content": "env-cmd --silent cross-env CONTENT_ROOT=files node node_modules/@mdn/yari/tool/cli.js",
-    "build": "env-cmd --silent cross-env CONTENT_ROOT=files BUILD_OUT_ROOT=build node node_modules/@mdn/yari/build/cli.js"
+    "build": "env-cmd --silent cross-env CONTENT_ROOT=files BUILD_OUT_ROOT=build node node_modules/@mdn/yari/build/cli.js",
+    "lint:md": "markdownlint \"**/*.md\" -i node_modules"


Okay, makes sense then. Maybe linting JavaScript and Markdown could be integrated via ESLint (eslint-plugin-markdownlint + eslint-plugin-prettier) in the future.

teoli2003 · 2022-08-23T06:57:29Z

Let's try this! (Especially as we can disable it very easily in case of a problem, so let's try it now in the morning :-) )

nschonni requested review from a team and fiji-flo and removed request for a team January 21, 2022 19:43

nschonni marked this pull request as draft February 3, 2022 05:15

nschonni force-pushed the markdownlint-dep branch from 4f744a6 to ac508e7 Compare February 24, 2022 01:35

nschonni marked this pull request as ready for review February 24, 2022 01:43

nschonni mentioned this pull request Feb 24, 2022

Add a Markdown linter for all .md files #10787

Closed

7 tasks

nschonni force-pushed the markdownlint-dep branch from fbcdb3f to a9342b7 Compare March 18, 2022 20:22

nschonni commented Mar 21, 2022

View reviewed changes

.github/workflows/markdown-lint.yml Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

nschonni marked this pull request as draft March 22, 2022 01:23

nschonni force-pushed the markdownlint-dep branch from 7636f2b to a02af35 Compare March 22, 2022 01:45

nschonni marked this pull request as ready for review March 22, 2022 01:48

nschonni commented Mar 22, 2022

View reviewed changes

nschonni force-pushed the markdownlint-dep branch from 0765df3 to ac74bb6 Compare March 24, 2022 15:30

nschonni force-pushed the markdownlint-dep branch from ac74bb6 to 6c82109 Compare March 30, 2022 00:31

OnkarRuikar reviewed Aug 18, 2022

View reviewed changes

.github/workflows/markdownlint-problem-matcher.json Show resolved Hide resolved

OnkarRuikar reviewed Aug 18, 2022

View reviewed changes

nschonni force-pushed the markdownlint-dep branch from 2ecd7db to 396a996 Compare August 18, 2022 14:29

OnkarRuikar mentioned this pull request Aug 19, 2022

Daily markdownlint #19718

Merged

nschonni added 5 commits August 20, 2022 19:35

chore: make Markdownlint a dependency and script

e64fcbb

ci: add markdownlint problem matcher

8d39243

chore: expand Markdownlint glob to all files

d270e8e

chore: add fix:md target to fix Markdownlint

8ea649a

chore: upgrade markdownlint-cli 0.32.2

c9bee3c

Disable new failing rules

nschonni force-pushed the markdownlint-dep branch from 396a996 to c9bee3c Compare August 20, 2022 23:37

caugner approved these changes Aug 22, 2022

View reviewed changes

teoli2003 merged commit 153d5a2 into mdn:main Aug 23, 2022

OnkarRuikar mentioned this pull request Aug 23, 2022

Fix definition lists #19863

Merged

nschonni deleted the markdownlint-dep branch August 23, 2022 13:57

OnkarRuikar mentioned this pull request Nov 24, 2022

ci: sync Markdownlint setup from mdn/content mdn/translated-content#8181

Merged

chore: make Markdownlint a dependency and script #12316

chore: make Markdownlint a dependency and script #12316

Conversation

nschonni commented Jan 21, 2022

nschonni commented Feb 3, 2022

nschonni commented Mar 18, 2022

hamishwillee commented Mar 21, 2022

nschonni commented Mar 21, 2022

nschonni left a comment

Choose a reason for hiding this comment

nschonni commented Mar 22, 2022

hamishwillee commented Mar 22, 2022

nschonni commented Mar 22, 2022

hamishwillee commented Mar 22, 2022 • edited Loading

nschonni commented Mar 22, 2022

nschonni commented Mar 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nschonni commented Mar 23, 2022

hamishwillee commented Mar 24, 2022

nschonni commented Mar 24, 2022

wbamberg commented Mar 25, 2022

wbamberg commented Mar 29, 2022

nschonni commented Aug 17, 2022

OnkarRuikar commented Aug 18, 2022 • edited Loading

teoli2003 commented Aug 18, 2022

teoli2003 commented Aug 18, 2022

OnkarRuikar commented Aug 18, 2022 • edited Loading

teoli2003 commented Aug 18, 2022

OnkarRuikar commented Aug 18, 2022 • edited Loading

teoli2003 commented Aug 18, 2022

teoli2003 commented Aug 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nschonni commented Aug 18, 2022

teoli2003 commented Aug 19, 2022 • edited Loading

OnkarRuikar commented Aug 19, 2022 • edited Loading

Lint issues as warning annotations instead of errors:

Green Merge pull request button despite the warnings:

teoli2003 commented Aug 19, 2022

nschonni commented Aug 19, 2022

caugner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teoli2003 commented Aug 23, 2022 • edited Loading

hamishwillee commented Mar 22, 2022 •

edited

Loading

OnkarRuikar commented Aug 18, 2022 •

edited

Loading

OnkarRuikar commented Aug 18, 2022 •

edited

Loading

OnkarRuikar commented Aug 18, 2022 •

edited

Loading

teoli2003 commented Aug 19, 2022 •

edited

Loading

OnkarRuikar commented Aug 19, 2022 •

edited

Loading

Green `Merge pull request` button despite the warnings:

teoli2003 commented Aug 23, 2022 •

edited

Loading