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 #2234

Merged
merged 10 commits into from
May 30, 2024

Conversation

trentm
Copy link
Contributor

@trentm trentm commented 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
Refs: #2207


The same was done in the core repo in open-telemetry/opentelemetry-js#4713

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
Copy link
Contributor Author

trentm commented May 23, 2024

This updated markdown linting actually catches the errant ### Dependencies sections described in #2207.

Copy link

codecov bot commented May 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.38%. Comparing base (dfb2dff) to head (8dba7f6).
Report is 138 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2234      +/-   ##
==========================================
- Coverage   90.97%   90.38%   -0.60%     
==========================================
  Files         146      147       +1     
  Lines        7492     7506      +14     
  Branches     1502     1573      +71     
==========================================
- Hits         6816     6784      -32     
- Misses        676      722      +46     

see 46 files with indirect coverage changes

Comment on lines 9 to 11
// XXX Rules to discuss:
"MD012": false // no-multiple-blanks, common in CHANGELOG.md files, ~1700 hits in changelog files
// "MD004": { "style": "dash" } // ul-style, ~3800 hits in 63 changelog files, XXX discuss just using "consistent" new default for dash
Copy link
Contributor Author

@trentm trentm May 23, 2024

Choose a reason for hiding this comment

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

@blumamir I'd like to discuss these two rules.

"MD012": false

This is not disabled in the core repo. If re-enabled in the contrib repo here, there are approximately 1700 hits (a) in most of the 63 CHANGELOG.md files and (b) a few hits in these files:

   1 plugins/node/opentelemetry-instrumentation-mysql/README.md
   3 .github/ISSUE_TEMPLATE/plugin_request.md
   4 .github/ISSUE_TEMPLATE/feature_request.md
   6 .github/ISSUE_TEMPLATE/bug_report.md

I think the cases in the CHANGELOG.md files will just keep happening because they are generated by release-please, we do not want this rule enabled for them. There are two options:

  1. globally disable the rule in this config file
  2. locally disable the rule for each CHANGELOG.md file. This is slightly burdensome (just a onetime edit) because there are 63 changelog files.

Thoughts?

MD004

Earlier we changed to style: "dash" to prefer dashes for bullets. In the core repo we disabled this rule locally in the 2 changelog files. Options:

  1. Disable it locally in the 63 changelog files -- similar to how we did in the core repo.
  2. Use the newer default of "consistent", which ensures that each file is consistent in the bullets it uses. That means the rule could apply to CHANGELOG.md files as well, but would enforce consistent * usage.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I think the cases in the CHANGELOG.md files will just keep happening because they are generated by release-please, we do not want this rule enabled for them. There are two options:

  1. globally disable the rule in this config file
  2. locally disable the rule for each CHANGELOG.md file. This is slightly burdensome (just a onetime edit) because there are 63 changelog files.

Thoughts?

Another alternative would be to use 2 markdown configs, one for CHANGELOG and one for other markdowns. I don't mind disabling it across the repo, but could be nice to have it if not too complex.

Earlier we changed to style: "dash" to prefer dashes for bullets. In the core repo we disabled this rule locally in the 2 changelog files. Options:

If we have different config file for CHANGELOGs than this is addressed as well, but if not straight-forward than we can disable it locally in each file or use the "consistent" option. What ever is easier and makes more sense to you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer to not have two separate config files, because:

  • it would be more likely for those config files to diverge,
  • it is a little messy to have yet another config file at the top,
  • it will mean markdownlint editor plugins (like the VSCode one referred to above) won't work properly -- the plugin will just use the default .markdownlint-cli2.jsonc file

In commit 2e03f59 I've opted to avoid <!-- markdownlist-disable ... --> directives in every CHANGELOG.md file by selecting options that work for all .md files. That means ul-style: consistent and no-multiple-blanks: false.

(A personal preference note: I like using multiple blank lines sometimes, both in Markdown and code, to separate bigger sections vs. a single line for related blocks. So I was never a fan of no-multiple-blank lines and the eslint equivalent in code.)

…CHANGELOG.md file and choose options that work for all .md files
@trentm trentm marked this pull request as ready for review May 23, 2024 14:36
@trentm trentm requested review from a team and blumamir May 23, 2024 14:36
@trentm trentm requested a review from blumamir May 27, 2024 16:08
@trentm trentm enabled auto-merge (squash) May 30, 2024 19:18
@trentm trentm merged commit 0078d0c into open-telemetry:main May 30, 2024
8 checks passed
@trentm trentm deleted the tm-use-markdownlint-cli2 branch May 30, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment