Add a Markdown linter for all .md files #211

Elchi3 · 2021-11-26T11:36:23Z

Elchi3
Nov 26, 2021
Maintainer

In addition to Prettier (see mdn/content#10402) it would be great to enable a linter that checks if the markdown makes sense.

There already is a markdown linter in CI, but it is currently enabled for meta docs only. mdn/content#7146 is a stalled PR that tried to enable it for all .md files.

Not all files currently comply with all md-lint rules. The following configuration would currently pass CI, but we should enable many of these rules (and potentially others not listed below).

To test, run npx markdownlint-cli . -i node_modules -i LICENSE.md and add .markdownlint.json to the content folder. Remove some of the lines below to see how they would fail.

{
    "default": true,
    "MD001": false, // Heading levels should only increment by one level at a time
    "MD004": false, // Consistent unordered list style
    "MD005": false, // Inconsistent indentation for list items at the same level
    "MD007": false, // Unordered list indentation
    "MD009": false, // Trailing spaces
    "MD010": false, //  Hard tabs
    "MD011": false, // Reversed link syntax
    "MD012": false, // Multiple consecutive blank lines
    "MD013": false, // Line length
    "MD014": false, // Dollar signs used before commands without showing output
    "MD019": false, // Multiple spaces after hash on atx style heading
    "MD022": false, // Headings should be surrounded by blank lines
    "MD023": false, // Headings must start at the beginning of the line
    "MD024": false, // Multiple headings with the same content
    "MD026": false, // Trailing punctuation in heading
    "MD027": false, // Multiple spaces after blockquote symbol
    "MD028": false, // Blank line inside blockquote
    "MD029": false, // Ordered list item prefix
    "MD030": false, // Spaces after list markers
    "MD031": false, // Fenced code blocks should be surrounded by blank lines
    "MD032": false, // Lists should be surrounded by blank lines
    "MD033": false, // Inline HTML
    "MD034": false, // Bare URL used
    "MD035": false, // Horizontal rule style
    "MD036": false, // Emphasis used instead of a heading
    "MD037": false, // Spaces inside emphasis markers
    "MD038": false, // Spaces inside code span elements
    "MD039": false, // Spaces inside link text
    "MD040": false, // Fenced code blocks should have a language specified
    "MD042": false, // No empty links
    "MD045": false, // Images should have alternate text (alt text)
    "MD046": false, // Code block style
    "MD047": false  // Files should end with a single newline character
  }

Tasks (roughly):

Fix files so that we can enable more MD rules
Document how to use markdownlint-cli locally (add yarn content mdlint command?)
Also add a way to use the --fix option easily (and document it, too)
Update PR authoring guide / contribution docs
Make sure CI reports linting errors in a beginner friendly way (should they become part of the bot message?)
Make sure markdownlint interacts sensibly with Prettier (and eslint)
Enable markdownlint-cli for all .md files

ddbeck · 2021-11-26T15:06:19Z

ddbeck
Nov 26, 2021

Fix files so that we can enable more MD rules

One thing to add here, from a discussion Florian and I had about this: PRs that fix these rules probably ought to be one rule at a time. Such PRs are going to be easier to review and it'll be more apparent if a rule can't be satisfied by our content or otherwise breaks our process.

0 replies

hamishwillee · 2021-12-06T02:34:12Z

hamishwillee
Dec 6, 2021

FYI, can we consider using the linter to catch use of non-US-English spellings. We fixed these everywhere relevant in mdn/content#373 , but I do catch myself using "real" English from time to time :-)

0 replies

ddbeck · 2021-12-29T10:53:17Z

ddbeck
Dec 29, 2021

@hamishwillee I think markdownlint only checks the source style for Markdown files and doesn't have a rule for spelling. I'd suggest opening an issue to track US spelling enforcement separately from Markdown source style.

0 replies

nschonni · 2022-01-28T03:35:40Z

nschonni
Jan 28, 2022

This is the currently passing config

{
  "default": true,
  "MD001": false,
  "MD004": false,
  "MD007": false,
  "MD013": false,
  "MD014": false,
  "MD023": false,
  "MD024": false,
  "MD025": {
    "front_matter_title": "^\\s*title\\s*[:=]"
  },
  "MD026": false,
  "MD028": false,
  "MD029": false,
  "MD030": false,
  "MD033": {
    "allowed_elements": [
      "a",
      "abbr",
      "annotation",
      "base",
      "br",
      "caption",
      "cite",
      "code",
      "col",
      "colgroup",
      "dd",
      "details",
      "dfn",
      "div",
      "dl",
      "dt",
      "em",
      "h4",
      "h5",
      "img",
      "kbd",
      "li",
      "math",
      "menclose",
      "mfenced",
      "mfrac",
      "mfrac",
      "mi",
      "mmultiscripts",
      "mn",
      "mo",
      "mover",
      "mphantom",
      "mprescripts",
      "mroot",
      "mrow",
      "ms",
      "mspace",
      "mspace",
      "msqrt",
      "mstyle",
      "msub",
      "msubsup",
      "msup",
      "mtable",
      "mtd",
      "mtext",
      "mtr",
      "munder",
      "munderover",
      "none",
      "ol",
      "p",
      "pre",
      "section",
      "semantics",
      "span",
      "strong",
      "sub",
      "summary",
      "sup",
      "table",
      "tbody",
      "td",
      "tfoot",
      "th",
      "thead",
      "tr",
      "ul",
      "var"
    ]
  },
  "MD034": false,
  "MD036": false,
  "MD037": false,
  "MD040": false,
  "MD042": false,
  "MD045": false,
  "MD046": {
    "style": "fenced"
  }
}

Does it make sense to add it now without integrating the full CI. That way it will work for those editing in IDEs with the markdownlint plugin?

0 replies

nschonni · 2022-01-28T03:38:25Z

nschonni
Jan 28, 2022

For spelling I usually use https://github.com/streetsidesoftware/cspell. My local dictionary is pretty big right now though

0 replies

hamishwillee · 2022-01-30T23:19:13Z

hamishwillee
Jan 30, 2022

@nschonni IMO better to embed a standard solution: mdn/content#12522. If we leave it up to individuals they won't bother. Case in point, me - US English makes me vomit. Or is that vomitz?

0 replies

nschonni · 2022-02-06T16:24:28Z

nschonni
Feb 6, 2022

There are 2 new rules around strong (MD050) and emphasis (MD049) markers. Does it make sense to keep the default of "consistent" or actually choose a style? I took a look at the rules
MD049:

asterisks: ~2400 changed files
underscore: ~300 changed files
consistent: ~140 files changed

MD050:

asterisks: ~20 files, but all are incorrect transforms of macros like {{jsxref("Object/**proto**", "__proto__")}}
underscore: ~4500 files changed
consistent: ~20 files changed. same as asterisks, but would need to tweak the others in the file to use underscore style so it wouldn't rewrite those same macros

0 replies

nschonni · 2022-02-06T16:29:00Z

nschonni
Feb 6, 2022

Updated config

{
  "default": true,
  "MD001": false,
  "MD004": {
    "style": "dash"
  },
  "MD007": {
    "indent": 2
  },
  "MD013": false,
  "MD014": false,
  "MD023": false,
  "MD024": false,
  "MD025": {
    "front_matter_title": "^\\s*title\\s*[:=]"
  },
  "MD026": false,
  "MD028": false,
  "MD033": {
    "allowed_elements": [
      "a",
      "abbr",
      "annotation",
      "base",
      "br",
      "caption",
      "cite",
      "code",
      "col",
      "colgroup",
      "dd",
      "details",
      "dfn",
      "div",
      "dl",
      "dt",
      "em",
      "h4",
      "h5",
      "img",
      "kbd",
      "li",
      "math",
      "menclose",
      "mfenced",
      "mfrac",
      "mfrac",
      "mi",
      "mmultiscripts",
      "mn",
      "mo",
      "mover",
      "mphantom",
      "mprescripts",
      "mroot",
      "mrow",
      "ms",
      "mspace",
      "mspace",
      "msqrt",
      "mstyle",
      "msub",
      "msubsup",
      "msup",
      "mtable",
      "mtd",
      "mtext",
      "mtr",
      "munder",
      "munderover",
      "none",
      "ol",
      "p",
      "pre",
      "section",
      "semantics",
      "span",
      "strong",
      "sub",
      "summary",
      "sup",
      "table",
      "tbody",
      "td",
      "tfoot",
      "th",
      "thead",
      "tr",
      "ul",
      "var"
    ]
  },
  "MD034": false,
  "MD036": false,
  "MD040": false,
  "MD042": false,
  "MD045": false,
  "MD046": {
    "style": "fenced"
  },
  "MD049": false,
  "MD050": false
}

MD037 seems unfixable for the same reason as the new rules. Underscores in the macros trip up the parsing, and that isn't something that upstream would address since it's outside of the CommonMark spec.

0 replies

hamishwillee · 2022-02-06T21:54:27Z

hamishwillee
Feb 6, 2022

Does it make sense to keep the default of "consistent" or actually choose a style?

FWIW No strong opinion, but if we do fix on a style I prefer using _ for emphasis and ** for bold as it is more clear. I lean towards keeping the current setting from a user point of view, since both a github compatible, and that is our baseline.

0 replies

nschonni · 2022-02-07T19:28:17Z

nschonni
Feb 7, 2022

I left those ones off for now and submitted a PR to at least get the current config in the root, without wiring it to the CI. Figure you folks may want to somehow integrate it into yari so you get some sort of integration with the "flaws"

0 replies

nschonni · 2022-02-24T01:47:29Z

nschonni
Feb 24, 2022

mdn/content#12316 is ready now for the next incremental step of having Markdownlint running a fixed version, with a GitHub Action problem matcher to highlight issues. It still only lints the root docs, and not the files content

0 replies

schalkneethling · 2022-05-26T12:03:12Z

schalkneethling
May 26, 2022

Hey @Elchi3, @hamishwillee, and @nschonni! There is this action used on translated-content. Could this be useful here and perhaps even solve the problem raised?

FYI, this is also being added to the mdn/workflows repo as a reusable action.

0 replies

nschonni · 2022-05-26T21:06:00Z

nschonni
May 26, 2022

The same job is in this repo https://github.com/mdn/content/blob/main/.github/workflows/markdown-lint.yml but both are "wrong" in that the globs only look at the root files. mdn/content#12316 is the PR to expand the glob so it hits the subfolders as well

0 replies

hamishwillee · 2022-05-27T01:19:53Z

hamishwillee
May 27, 2022

I'm not really sure what I'm being asked here. Isn't this being run on a daily basis somehow already?

@nschonni let me know if you need anything from me.

0 replies

teoli2003 · 2022-05-27T04:02:50Z

teoli2003
May 27, 2022

How many files would be changed if we ran this today? I would like to have a daily script, but I think we should be conservative with the number of rules, and when we want to add a rule, we, first, clean MDN for it and then add it.

I want to be sure we don't have 50+ pages in such PR when we start.

0 replies

nschonni · 2022-05-27T04:23:31Z

nschonni
May 27, 2022

How many files would be changed if we ran this today?

I didn't get any hits running it now as it's been cleaned regularly, even though the CI job doesn't test much of the files right now

I would like to have a daily script

That is what mdn/content#16036 is supposed to do, but has like-wise stalled

but I think we should be conservative with the number of rules, and when we want to add a rule, we, first, clean MDN for it and then add it.

I did that over a year ago when the first attempt to expand out the CI job stalled out. I've stopped trying to keep mdn/content#12316 in sync as it has also stalled

0 replies

teoli2003 · 2022-05-27T04:58:19Z

teoli2003
May 27, 2022

So this is what I believe we should do:

For each rule we want to add, we should clean MDN first (no point in having large PRs to review every day)
We should run a daily script automatically fixing rules that are broken (like no empty space at the end of the file, or adding an extra space at the end of the file if none). Only automatically fixable rules should be checked for with this action.
For another set than the rules in 2., lint each PR: I don't see a need to enforce no empty space at EOL for each PR. The burden for new contributors will be high (especially via the GitHub interface), but I can see us enforcing fenced code block only there because it is rarer and somebody using it is already fairly MD-savvy and not just copying one of our templates. Also, rules that cannot automatically be fixed can be checked there.

I think 1. is mostly done and we should start with 2. for some time to see how it goes: The action @schalkneethling linked above would perform 2. (I have not looked at the linter config file, though). If I understand well this very PR is for 3. and #16036 is for 2, too.

Overall let's be conservative with the rules we check for or enforce, it will be easy to add more later, in separate PRs: no need to argue forever before landing the action/linting to have the perfect set of rules. Let's go for minimum sets and extend them afterward.

I would add that @OnkarRuikar is manually running such a linter daily for a week, leading to fixing 5-10 pages per day, mostly for empty spaces, which is easy and quick to review.

@wbamberg wdyt?

0 replies

hamishwillee · 2022-05-27T05:01:03Z

hamishwillee
May 27, 2022

The only thing I'd add to ^^^ is perhaps (if possible) we auto-limit the max size of a PR that the linter will throw out. We certainly shouldn't be stalled - either we move on or we cease.

0 replies

OnkarRuikar · 2022-05-27T06:08:53Z

OnkarRuikar
May 27, 2022

The issues which the linter can't fix(e.g. broken header hierarchy) needs to be done manually. So, whichever mechanism we use needs to report such issues as a comment or logs somewhere.

@schalkneethling if we decide to enforce 64/80 line length for the the PR, there will be a huge number of files flagged by the linter. And the linter won't fix them.

FYI I use the following command:

markdownlint "./files/**/*.md" "./meta/content/**/*.md" --fix

It uses the config file, available at the root of the repo.

0 replies

hamishwillee · 2022-05-27T06:58:05Z

hamishwillee
May 27, 2022

We do not want to enforce a line length on markdown. I abhor them.

0 replies

wbamberg · 2022-05-27T15:51:30Z

wbamberg
May 27, 2022
Maintainer

Yes, we had this conversation already and the consensus was pretty clear! mdn/content#6936.

0 replies

schalkneethling · 2022-05-31T19:20:50Z

schalkneethling
May 31, 2022

Yes, we had this conversation already and the consensus was pretty clear! #6936.

Yes, so it is clear we do not want line wrapping and we would prefer a custom Prettier config that sets preserve to prose-wrapping as suggested here: mdn/content#6936 (comment)

@wbamberg What would be the action items to get us to a place where we can close this issue? (BTW, not meant as a push to make a decision, more a way for me to understand what still needs doing.) Thanks!

0 replies

nschonni · 2022-06-16T19:48:02Z

nschonni
Jun 16, 2022

It would be good to get a review on mdn/content#16036. The listing of files across actions runs wasn't possible (for me), but it will indicate to maintainers to look at any failed run for the unfixable things on a daily run

0 replies

Rumyra · 2022-08-26T10:03:37Z

Rumyra
Aug 26, 2022
Maintainer

Converting to a discussion 👍

0 replies

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MDN Web Docs

Add a Markdown linter for all .md files #211

{{title}}

{{editor}}'s edit

{{editor}}'s edit

Replies: 0 comments 24 replies

{{title}}

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{title}}

{{title}}

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

Select a reply

Add a Markdown linter for all .md files #211

Elchi3 Nov 26, 2021 Maintainer

Replies: 0 comments · 24 replies

wbamberg May 27, 2022 Maintainer

Rumyra Aug 26, 2022 Maintainer

Elchi3
Nov 26, 2021
Maintainer

Replies: 0 comments 24 replies

wbamberg
May 27, 2022
Maintainer

Rumyra
Aug 26, 2022
Maintainer