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

chore(lint): refactor Markdown linting to use markdownlint-cli2 #4713

Merged
merged 10 commits into from
May 21, 2024

Conversation

trentm
Copy link
Contributor

@trentm trentm commented May 16, 2024

This switches from markdownlint-cli to markdownlint-cli2 and drops
usage of the https://github.com/avto-dev/markdown-lint GitHub action in CI.

  • The avto-dev/markdown-lint action was using a 4y old version of markdownlint.
    AFAICT that action is not being maintained.
  • The lint.yml workflow now uses npm run lint:markdown so that linting in CI
    is the same as doing so in dev.
  • The switch from markdownlint-cli to markdownlint-cli2 and .markdownlint-cli2.jsonc
    as the config allows using the https://marketplace.visualstudio.com/items?itemName=DavidAnson.vscode-markdownlint
    VSCode plugin for in-editor Markdown lint warnings and intellisense in the config file.
  • This also changes to prefer dash for ul-style (MD004), except in CHANGELOG.md files.

Refs: #4703


This is an alternative to #4703
/cc @blumamir

on ul-style

#4698 discusses whether to prefer a particular bullet character for unordered lists in Markdown. This corresponds to the MD004 rule. The default value is "consistent" ... meaning that the first bullet style in a file dictates the preference for the remainder of that file.

Currently this PR includes two commits to change to prefer the dash style... with the exception of the 3 CHANGELOG.md files which ignore this rule (via a <!-- markdownlint-disable MD004 --> directive).

(The CHANGELOG.md files are skipped because they consistently use *, including new content automatically added by tools such as release-please.)

separate commits

This PR is separated into a number of commits to try to separate the config changes from the lint-update changes, in case that helps with reviewing.

@trentm trentm self-assigned this May 16, 2024
@trentm trentm requested a review from a team as a code owner May 16, 2024 19:25
@trentm
Copy link
Contributor Author

trentm commented May 16, 2024

special changelog lint rules in avto-dev/markdown-lint

One of the potential pitches of the avto-dev/markdown-lint action is that it supported some addition lint rules for changelog files.

However, my understanding is that we have never used those rules because they are commented out here:

# Commenting due to
# https://github.com/avto-dev/markdown-lint/blob/aab9be9823fcd706f6da3e6ad6c64874c23fbd4c/lint/rules/changelog.js#L51-L71
# TODO: adhere to, or overwrite above rule and uncomment rules
# rules: "/lint/rules/changelog.js"

So, nothing is lost by switching away from this GH Action.

Copy link

codecov bot commented May 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.57%. Comparing base (2610122) to head (397e001).
Report is 36 commits behind head on main.

Current head 397e001 differs from pull request most recent head c6753bb

Please upload reports for the commit c6753bb to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4713      +/-   ##
==========================================
+ Coverage   90.77%   93.57%   +2.79%     
==========================================
  Files          90      298     +208     
  Lines        1930     8466    +6536     
  Branches      417     1753    +1336     
==========================================
+ Hits         1752     7922    +6170     
- Misses        178      544     +366     

see 244 files with indirect coverage changes

@trentm
Copy link
Contributor Author

trentm commented May 16, 2024

For the contrib repo, open-telemetry/opentelemetry-js-contrib#2205 was a recent change adding "dash" ul-style. If this PR is amenable I can make a PR to switch the contrib repo over to markdownlint-cli2 as well.

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

Thanks @trentm great research and digging, happy to see up-to-date tolling and better developer experience.

left few minor questions / thoughts

.markdownlint-cli2.jsonc Show resolved Hide resolved
.markdownlint-cli2.jsonc Show resolved Hide resolved
.markdownlint-cli2.jsonc Show resolved Hide resolved
.markdownlint-cli2.jsonc Show resolved Hide resolved
.markdownlint-cli2.jsonc Show resolved Hide resolved
@blumamir
Copy link
Member

For the contrib repo, open-telemetry/opentelemetry-js-contrib#2205 was a recent change adding "dash" ul-style. If this PR is amenable I can make a PR to switch the contrib repo over to markdownlint-cli2 as well.

That sounds great, would love to see this change in contrib as well 🥇

@trentm trentm requested a review from blumamir May 17, 2024 18:45
@pichlermarc pichlermarc merged commit e49c4c7 into open-telemetry:main May 21, 2024
18 checks passed
@trentm trentm deleted the tm-markdown-lint branch May 22, 2024 14:17
trentm added a commit to trentm/opentelemetry-js-contrib that referenced this pull request May 23, 2024
This switches from `markdownlint-cli` to `markdownlint-cli2` and drops
usage of the https://github.com/avto-dev/markdown-lint GitHub action in CI.

- The `avto-dev/markdown-lint` action was using a 4y old version of `markdownlint`.
  AFAICT that action is not being maintained.
- There is a new `npm run lint:markdown` and the `lint.yml` CI workflow uses it.
- The switch from `markdownlint-cli` to `markdownlint-cli2` and `.markdownlint-cli2.jsonc`
  as the config allows using the https://marketplace.visualstudio.com/items?itemName=DavidAnson.vscode-markdownlint
  VSCode plugin for in-editor Markdown lint warnings and intellisense in the config file.

Refs: open-telemetry/opentelemetry-js#4713
trentm added a commit to open-telemetry/opentelemetry-js-contrib that referenced this pull request May 30, 2024
This switches from `markdownlint-cli` to `markdownlint-cli2` and drops
usage of the https://github.com/avto-dev/markdown-lint GitHub action in CI.

- The `avto-dev/markdown-lint` action was using a 4y old version of `markdownlint`.
  AFAICT that action is not being maintained.
- There is a new `npm run lint:markdown` and the `lint.yml` CI workflow uses it.
- The switch from `markdownlint-cli` to `markdownlint-cli2` and `.markdownlint-cli2.jsonc`
  as the config allows using the https://marketplace.visualstudio.com/items?itemName=DavidAnson.vscode-markdownlint
  VSCode plugin for in-editor Markdown lint warnings and intellisense in the config file.
- remove errant Dependencies sections from the top of some changelog files found by rule MD001 (see #2207)

Refs: open-telemetry/opentelemetry-js#4713
Closes: #2207
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.

None yet

3 participants