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

Add MarkdownLint CLI checker #1366

Merged
merged 1 commit into from
Feb 4, 2018

Conversation

dholm
Copy link
Contributor

@dholm dholm commented Nov 26, 2017

Adds support for the MarkdownLint Command Line Interface based on the
Node.js markdownlint package.

@CLAassistant
Copy link

CLAassistant commented Nov 26, 2017

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@cpitclaudel cpitclaudel left a comment

Choose a reason for hiding this comment

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

Looks very good :) Thanks! Could you add an entry in CHANGES?

@dholm
Copy link
Contributor Author

dholm commented Nov 26, 2017

@cpitclaudel Done!

@cpitclaudel
Copy link
Member

Thanks! One last thing: you should use .. syntax-checker-config-file instead of .. defcustom for the config file (that's what causing the build to fail)

@dholm
Copy link
Contributor Author

dholm commented Nov 26, 2017

Oops, fixed!

CHANGES.rst Outdated
=======================
=========================
32-cvs (in development)
=========================
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it was necessary to restyle every headings. But I guess it's your mode that made the change sneakily?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, yes, I didn't even catch that myself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@fmdkdd
Copy link
Member

fmdkdd commented Dec 11, 2017

Great! We just need you to squash your commits before we can merge :)

@dholm
Copy link
Contributor Author

dholm commented Dec 13, 2017

@fmdkdd Memo to self, never edit code on GitHub. :/

Adds support for the MarkdownLint Command Line Interface based on the
Node.js markdownlint package.

 - https://github.com/igorshubovych/markdownlint-cli
@fmdkdd
Copy link
Member

fmdkdd commented Dec 14, 2017

@dholm Thanks. One last thing: how does markdown-cli compare to mdl, the other linter we already have in Flycheck? I want to know if it's worth changing the default to markdown-cli, or whether we should keep mdl (because it has more lints/has better lints/is more performant/more popular or whatever).

@dholm
Copy link
Contributor Author

dholm commented Dec 19, 2017

@fmdkdd I used mdl at first but had issues with suppressions not working according to the documentation, for instance disabling the line-length check for tables. markdownlint-cli appears to be better maintained and did not have these issues.

@fmdkdd
Copy link
Member

fmdkdd commented Dec 19, 2017

@dholm I see, thank you.

@marsam You added mdl, do you have an opinion on whether we should switch to markdownlint-cli as the default checker for Markdown?

@cpitclaudel
Copy link
Member

cpitclaudel commented Feb 4, 2018

Ping @marsam , any opinions on changing the default checker? :)

@marsam
Copy link
Contributor

marsam commented Feb 4, 2018

Sorry for the late response, I don't have any issues with changing markdownlint-cli as default checker

@cpitclaudel cpitclaudel merged commit dff036e into flycheck:master Feb 4, 2018
@cpitclaudel
Copy link
Member

@marsam Merged, thanks @dholm! (And sorry for the delay) Let's change the default checker in a separate PR. @dholm, would you like to open one?

@dholm
Copy link
Contributor Author

dholm commented Feb 14, 2018

@cpitclaudel Thank you! Isn't it already the default though? I thought the order in flycheck-checkers is what makes it default.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants