Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lint the markdown, closes #3172 #3173

Merged
merged 1 commit into from Jan 3, 2018
Merged

lint the markdown, closes #3172 #3173

merged 1 commit into from Jan 3, 2018

Conversation

boneskull
Copy link
Member

@boneskull boneskull commented Dec 21, 2017

add @mocha/markdownlint-cli; closes #3172

  • forked markdownlint-cli to fix a glob-related bug; will abandon our version if/when it's merged
  • add .markdownlint.json config
  • remove "allow trailing spaces" for Markdown in .editorconfig
  • add to lint target & create markdownlint script in package.json, which is dumb, but we're stuck with it for now
  • linted the markdown

@boneskull
Copy link
Member Author

the worst was probably the changelog...

@boneskull
Copy link
Member Author

(netlify builds will fail until #3170 is merged)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 89.968% when pulling c240edc on markdownlint into 9150e34 on master.

Copy link
Contributor

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome! why not use remark-cli instead, i've seen it working superbly for the node-core project. remark configs are very similar to eslint configs.

It has some cool features:

  • Loads remark- plugins
  • Searches for markdown extensions
  • Ignores paths found in .remarkignore files
  • Loads configuration from .remarkrc, .remarkrc.js files
  • Uses configuration from remarkConfig fields in package.json files

Also i think adding an 80 line length limit would make the md files a lot neater.

@boneskull
Copy link
Member Author

@Bamieh Both remark-cli and markdownlint-cli seem to have about the same amount of adoption, so I picked the one which seemed more straightforward. All we need is linting, after all. Conversion of markdown to HTML is done with kramdown by way of Jekyll.

Given that, why would Mocha need remark and its plugins?

Generally I'm in agreement on the line length, but I think there was a reason I didn't enforce this. I just don't remember what it is... I'll take another look and see if there are any speed bumps here.

@boneskull
Copy link
Member Author

I've rebased this onto master so it's synced with @Munter's changes.

@Munter This mainly looked OK, but I ended up doing two things:

  1. Replaced some HTML with Markdown in the nav tag.
  2. Reverted a change to a link you made (https ➡️ http) due to broken SSL on a site

@boneskull boneskull added area: documentation anything involving docs or mochajs.org semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Dec 29, 2017
@boneskull
Copy link
Member Author

...looks like one of my changes broke.

@boneskull
Copy link
Member Author

That Netlify preview is pretty awesome.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 89.968% when pulling 98a8313 on markdownlint into adc67fd on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 89.968% when pulling 564b681 on markdownlint into adc67fd on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 89.968% when pulling c22aad8 on markdownlint into adc67fd on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 89.973% when pulling df323e7 on markdownlint into a554adb on master.

@Bamieh
Copy link
Contributor

Bamieh commented Jan 3, 2018

@boneskull I suggested remark for mainly because you had to fork markdownlint to get it to work, so i suggested an alternative that i personally favor due to its awesome extendability. Given the current state of the PR, I believe there is no need for such change.

We can enforce the 80 line length rule later on as the project sees fit.

Awesome work! LGTM

- add [markdownlint-cli](https://npm.im/markdownlint-cli)
- lint all Markdown files
- add some rules to `.markdownlintrc`
Copy link
Contributor

@markowsiak markowsiak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚢

@coveralls
Copy link

Coverage Status

Coverage remained the same at 89.973% when pulling b61e91b on markdownlint into dc12bd5 on master.

@boneskull
Copy link
Member Author

markdownlint-cli accepted my PR and published, so we're using that instead of our fork.

@boneskull boneskull merged commit a723b8f into master Jan 3, 2018
@boneskull boneskull deleted the markdownlint branch January 3, 2018 22:58
@boneskull boneskull added this to the v5.0.0 milestone Jan 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: documentation anything involving docs or mochajs.org semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lint the markdown
5 participants