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

ci: markdown linting cleanups #4703

Closed
wants to merge 13 commits into from
Closed

Conversation

blumamir
Copy link
Member

@blumamir blumamir commented May 14, 2024

This PR includes the following fixes:

  • apply Lint changelog file CI step to all markdowns and not only the one in the repo root. This introduced few fixes to existing CHANGELOG files which now failed to lint
  • exclude all CHANGELOG.md files from the next ci step Lint markdown files, which makes it consistent with contrib
  • align all our markdowns to common style - dash for unordered lists in markdown.
  • added rule to CI to enforce the list marker style
  • cleanup irrelevant paths from the "ignroe" option in the ci step config for markdown-lint. they refer to times when the exporters where part of the packages directory and not under experimental.

see #4698 and the equivalent contrib PR: open-telemetry/opentelemetry-js-contrib#2205

@blumamir blumamir requested a review from a team May 14, 2024 05:51
@blumamir blumamir changed the title ci: add md linter rule to only use dash for unordered lists ci: markdown linting cleanups May 14, 2024
.github/workflows/lint.yml Outdated Show resolved Hide resolved
Comment on lines -123 to +125
1. A user extends the exporter and overrides the shutdown function, and does something which is usually called by the unload listener
2. We remove the unload event listener
3. That user's overridden shutdown function no longer gets called
* A user extends the exporter and overrides the shutdown function, and does something which is usually called by the unload listener
* We remove the unload event listener
* That user's overridden shutdown function no longer gets called
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this change intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, now that this file participates in the CHANGLELOG linting in CI, the linter emitted errors about it

Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm. That feels like a bug in markdown-lint then. Is an ordered list under an unordered list not allowed?

Copy link
Member

Choose a reason for hiding this comment

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

I was trying to find proper guidance and struggling a bit. My thought is generally to follow what HTML allows, which is that a list can be ordered or unordered, not mixed. Looks like GitHub Flavored Markdown spec suggests:

A list is a sequence of one or more list items of the same type. The list items may be separated by any number of blank lines.

So I think technically ordered sub-items should not exist in an unordered list.

Copy link
Contributor

Choose a reason for hiding this comment

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

My thought is generally to follow what HTML allows, which is that a list can be ordered or unordered, not mixed.

IIUC, this is about the list item in an unordered list containing an ordered list.
Like this: https://gist.github.com/trentm/ab0df8f9ded43ec474d5f06b4db3ab6d

Markdown and HTML allow this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Playing with markdown-lint separately (npm install markdown-lint, ./node_modules/.bin/markdown-lint foo.md):

It is okay with this:

-   one

-   two

-   three

    1. un
    1. deux
    1. trois

It doesn't like the sane bullet numbering in this:

-   one

-   two

-   three

    1. un
    2. deux
    3. trois
% ./node_modules/.bin/markdown-lint foo.md
<stdin>
  8:5-8:12  warning  Marker should be `1`, was `2`  ordered-list-marker-value  remark-lint
  9:5-9:13  warning  Marker should be `1`, was `3`  ordered-list-marker-value  remark-lint

⚠ 2 warnings

It is also over picky (IMHO) with "tight" lists (as GFM calls them, https://github.github.com/gfm/#tight):

- one
- two
- three
    1. un
    1. deux
    1. trois
% ./node_modules/.bin/markdown-lint foo.md
<stdin>
  1:6-2:1  warning  Missing new line after list item  list-item-spacing  remark-lint
  2:6-3:1  warning  Missing new line after list item  list-item-spacing  remark-lint

⚠ 2 warnings

@blumamir If you recall, was the markdown-lint error about the numbering of bullets, do you recall?

Marker should be `1`, was `3`  ordered-list-marker-value

The markdown-lint github action being used here says it is "with presets", but I couldn't quickly find what those preset were. markdown-lint's default config is too picky.

Anyway, I realize I am well into overkill here. :) I'll approve this PR. It helps and this single instance isn't this time-important.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was the lint error:

https://github.com/open-telemetry/opentelemetry-js/actions/runs/9074537658/job/24933472224

./experimental/CHANGELOG.md:124:1 MD029/ol-prefix Ordered list item prefix [Expected: 1; Actual: 2; Style: 1/1/1]
./experimental/CHANGELOG.md:125:1 MD029/ol-prefix Ordered list item prefix [Expected: 1; Actual: 3; Style: 1/1/1]

Do you suggest converting it to 1. 1. 1. instead of * * *?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nerd-me would see about disabling that MD029/ol-prefix check in the markdown-lint settings.

Personally I think it is a misguided option. The idea is that the maintenance of writing ordered-lists in Markdown is reduced if all the bullets are 1. because you don't need to re-number if you add or remove list items. But it comes at the cost of being able to unambiguously refer to "item N" when reading the source Markdown, instead of the rendered view.

Ah, it looks like this changed in more recent versions of markdownlint to default to one_or_ordered, so that either is fine by default: DavidAnson/markdownlint#97

This got me digging a bit. It looks like the markdownlint-cli being used in avto-dev/markdown-lint GH action is about 4y old. That github action isn't being maintained. As well, we have a npm run lint:markdown that behaves slightly differently than the markdown linting being done in the lint.yml GH workflow.

I'm going to open a separate PR as an alternative.
...
Opened here: #4713

Copy link
Contributor

Choose a reason for hiding this comment

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

(Also, as an aside, my earlier comments showing usage of npm install markdown-lint could be misleading, because the repo is currently using markdownlint-cli, which uses markdownlint. These are both unrelated to the markdown-lint that I used. Confusing.)

Co-authored-by: Trent Mick <trentm@gmail.com>
@blumamir
Copy link
Member Author

Closing in favor of #4713 which also updating the tool we use and introduces package.json scripts to do the lint consistently. Thank you trent for doing the research and adding improvements

@blumamir blumamir closed this May 17, 2024
pichlermarc added a commit that referenced this pull request May 21, 2024
* chore(lint): refactor Markdown linting to use markdownlint-cli2

- first commit is just config changes; lint updates will follow

* lint fixes

* fix markdownlint for rule MD045/no-alt-text

* lint config changes for prefering 'dash' style for rule MD004/ul-style

* lint:markdown:fix changes for MD004/ul-style

* manually apply this h3->h2 fix that Amir had in his #4703 PR

* mention markdown linting in the Linting section of the contributor guide

* add link to rules docs

---------

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…-telemetry#4713)

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

- first commit is just config changes; lint updates will follow

* lint fixes

* fix markdownlint for rule MD045/no-alt-text

* lint config changes for prefering 'dash' style for rule MD004/ul-style

* lint:markdown:fix changes for MD004/ul-style

* manually apply this h3->h2 fix that Amir had in his open-telemetry#4703 PR

* mention markdown linting in the Linting section of the contributor guide

* add link to rules docs

---------

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
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