-
Notifications
You must be signed in to change notification settings - Fork 837
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
Conversation
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this change intentional?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 * * *
?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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>
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 |
* 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>
…-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>
This PR includes the following fixes:
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 lintLint markdown files
, which makes it consistent with contribpackages
directory and not under experimental.see #4698 and the equivalent contrib PR: open-telemetry/opentelemetry-js-contrib#2205