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 to Circle CI config #268

Merged
merged 7 commits into from Oct 17, 2019

Conversation

songy23
Copy link
Member

@songy23 songy23 commented Sep 27, 2019

Fixes #267.

@songy23
Copy link
Member Author

songy23 commented Sep 27, 2019

Looks like it's working: https://circleci.com/gh/open-telemetry/opentelemetry-specification/150?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Now we need to fix all the lint errors :)

@bogdandrutu
Copy link
Member

bogdandrutu commented Sep 30, 2019

I am personally happy to have linter enabled to ensure style consistency. Anyone else please comment what do you think?

@c24t
Copy link
Member

c24t commented Sep 30, 2019

Good call on making the style consistent.

What's the reason for using markdownlint-cli instead of markdownlint? The environment has js but not ruby?

I'd prefer disabling the line length check and using soft line breaks everywhere as in #192.

@bogdandrutu
Copy link
Member

I think using the second markdownlint tool that @c24t mentioned may be better.

Why disabling the line length check?

Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

I like the linter.

@bogdandrutu
Copy link
Member

@SergeyKanzhelev @songy23 @c24t I am fine for the moment to disable the line length check, if that unblocks enabling this, and we can revisit that.

@bogdandrutu
Copy link
Member

@songy23 you can install the plugin for VScode with the same name and I think will be easier to fix all the errors.

@songy23
Copy link
Member Author

songy23 commented Oct 1, 2019

Unfortunately there's no easy way to auto-fix the lint errors even with the vs code extension. See DavidAnson/vscode-markdownlint#85. As of today the only way is to go through each lint error and manually fix them one by one, and given the amount of errors that would be non-trivial. I'd suggest waiting until DavidAnson/vscode-markdownlint#85 is implemented (looks like it's on their list).

@songy23 songy23 force-pushed the circle-ci-markdown branch 2 times, most recently from eb26ec0 to 77832f4 Compare October 1, 2019 18:49
Copy link
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

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

See songy23#1. I added a style file for myself while making edits in this repo and thought it would be useful here. This should scale more easily than adding flags to the CI command.

.circleci/config.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@tedsuo tedsuo left a comment

Choose a reason for hiding this comment

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

Nice.

@DavidAnson
Copy link

Hey all!

FYI, the latest version of markdownlint (Node version, published 2 days ago) includes fix information for 24 rules. I'll be updating the VS Code markdownlint extension to use it (enabling fix one-error-at-a-time) in the next release (within a week) and updating the markdownlint-cli package (enabling fix all-files-at-once) after that (maybe another week). The change to add fix-all-in-this-file to the VS Code extension comes later, but will be on a per-file basis anyway. You'll probably have the easiest time using markdownlint-cli once it's released. Fortunately, all 3 of these tools use the same .markdownlint.json format, so if you get things configured as you wish now, it will make the conversion a bit simpler. Note that this capability is new and required refactoring the 24 rules it affects, so there's a chance there will be issues. I'll be happy to work with you on any that you encounter!

@DavidAnson
Copy link

Another FYI, the Node started out trying to be 100% compatible with the Ruby implementation. That has drifted slightly over time as I have added features and expanded the Node implementation. I encourage you to use whichever you find more compelling - either way you can use the markdownlint-cli Node package as a one-off to fix what it can and save you the extra manual effort.

@DavidAnson
Copy link

It turns out, the next release of the VS Code markdownlint extension will support fix-all-in-current-document: DavidAnson/vscode-markdownlint@50794be

@songy23
Copy link
Member Author

songy23 commented Oct 15, 2019

Thanks @DavidAnson, I upgraded VS Code markdownlint extension to v0.31.0 and fixed most of the errors with the fixAll command. Super helpful!

Approvers - most errors are fixed but there're rules that are not auto-fixable for now, so I excluded a few more rules in 332f054. PTAL.

If this change looks good please disable ci/circleci: build (it's now split to two sub-tasks) and enable the other two checks:
Screen Shot 2019-10-15 at 1 42 06 PM

@SergeyKanzhelev
Copy link
Member

As discussed we wanted to ship a v0.2 today. Let's wait with this till tomorrow to not disturb in in-flight PRs

@SergeyKanzhelev
Copy link
Member

super cool

@SergeyKanzhelev SergeyKanzhelev added this to the Alpha v0.3 milestone Oct 16, 2019
@SergeyKanzhelev
Copy link
Member

@songy23 please re-apply all the fixes and I'll be happy to merge this

@songy23
Copy link
Member Author

songy23 commented Oct 17, 2019

Rebased and fixed, PTAL @SergeyKanzhelev.

@SergeyKanzhelev
Copy link
Member

SergeyKanzhelev commented Oct 17, 2019

I'll merge as soon as circleCI will report success.

Would you mind also add a note to CONTRIBUTING.md on this new requirement and how to auto-fix it as a separate PR

@songy23
Copy link
Member Author

songy23 commented Oct 17, 2019

@SergeyKanzhelev ci/circleci: build will be pending forever because in the updated config.yml, the rule build no longer exists (it's split to misspell and markdownlint), while it's still a required check for PR.

What we can do is

  • go to 'Settings' -> 'Branches' -> 'master' -> 'Require status checks to pass before merging'
  • uncheck ci/circleci: build
  • check ci/circleci: misspell and ci/circleci: markdownlint

I'll update CONTRIBUTING in a follow-up PR.

@SergeyKanzhelev
Copy link
Member

@songy23 entering the god mode....

@SergeyKanzhelev SergeyKanzhelev merged commit a131ae1 into open-telemetry:master Oct 17, 2019
@songy23
Copy link
Member Author

songy23 commented Oct 17, 2019

Thanks!

@songy23 songy23 deleted the circle-ci-markdown branch October 17, 2019 20:33
@songy23 songy23 mentioned this pull request Oct 18, 2019
SergeyKanzhelev pushed a commit to SergeyKanzhelev/opentelemetry-specification that referenced this pull request Feb 18, 2020
* Add markdownlint to Circle CI config

* Use markdownlint instead of markdownlint-cli

* Disable line length check (MD013)

* Add .mdlrc, .mdlstyle.rb

* Disable ordered list item (MD029)

* Use .mdlrc in circle ci scripts

* Fix errors and update exclusion rules
TuckTuckFloof pushed a commit to TuckTuckFloof/opentelemetry-specification that referenced this pull request Oct 15, 2020
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.

Suggest to enable markdownlint at the pull request time
6 participants