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

mdast-lint based markdown checking #67

Closed
wants to merge 3 commits into from
Closed

mdast-lint based markdown checking #67

wants to merge 3 commits into from

Conversation

tkruse
Copy link

@tkruse tkruse commented Nov 1, 2015

As suggested in #34, this PR introduces checking of the markdown files in articles/, failing the travis build when style errors are found.

Example output: https://travis-ci.org/tkruse/design/builds/88631913

The inner README.md file provides some info and instructions to run locally. The commands could be wrapped in shell or Makefile scripting, but I did not add that since I don't know your preferences.

Notes:

  • I would have preferred a python solution, you probably too, but this is the best I could find at the time.
  • I put the additional files into subfolders to allow easy copy&paste into other projects, putting everything into the project root would also work.
  • The mdast-lint configuration of the style rules are copied from another project I work on, so they probably need tweaking to your preferences. Note that for several current style items, the easiest solution is to run mdast to fix the files as explained in the README.md, by piping mdast output into the source file.
  • I had to add pygments.rb in Gemfile to get things to run on travis.
  • If you like this, consider mentioning this tool on the ROS2 style guide wiki page.

@dirk-thomas
Copy link
Member

Thank you for putting together the PR.

We should probably refer to a more complete style guide from our wiki page. Maybe http://www.cirosantilli.com/markdown-style-guide/ Based on that we can figure out if the configuration in this patch should be adjusted or if the existing markdown files should be updated.

There are two aspects to consider for selecting the linter to be used:

  1. Which linter is already being supported by popular editors. I found:
  2. Since neither of these two linters supports checking the "wrap-after-each-sentence" rule (http://www.cirosantilli.com/markdown-style-guide/#option-wrap-sentence) to which one can we contribute such a rule? Especially this rule is imo essential for the linter for us.

@wooorm
Copy link

wooorm commented Nov 3, 2015

I’d suggest mdast 😉

There’s also an external lint-rule for “option-wrap-sentence”: https://github.com/chcokr/mdast-lint-sentence-newline, which I think does exactly what you’d like?

@dirk-thomas
Copy link
Member

That is great. @wooorm thanks for pointing it out. I think that fact makes a strong vote for using mdast.

@tkruse Can you please integrate the external rule into this PR?

After that we can look at the difference between the current md files and the linter output and adjust either of them to pass.

@tkruse
Copy link
Author

tkruse commented Nov 3, 2015

added external rule

tfoote added a commit that referenced this pull request Nov 4, 2015
Trying out mdast checking from #67
@tfoote
Copy link
Contributor

tfoote commented Nov 4, 2015

I've tried this out locally in atom. (The linter-markdown requires updating .mdastrc and manually installing the sentence plugin into the ~/.atom/packages/linter-markdown)

There was one flag that I could not resolve which was that the use of . inside the jekyll markup was flagged by the sentance plugin. I've tickted it here, but there appear to already be several other special cases not caught ticketed in a backlog.

tfoote added a commit that referenced this pull request Nov 4, 2015
tfoote added a commit that referenced this pull request Nov 4, 2015
@dirk-thomas
Copy link
Member

Sadly we haven't taken any actions on this PR for a while and by now mdast has been renamed to remark. I have created a new PR #74 which replaces this one. It also updates the existing markdown files to pass the linter.

@dirk-thomas dirk-thomas closed this Jan 8, 2016
@tfoote tfoote mentioned this pull request Jan 8, 2016
34 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants