DavidAnson / markdownlint

A Node.js style checker and lint tool for Markdown/CommonMark files.
MIT License
4.71k stars 715 forks source link

MD018: do not check `#` in HTML blocks #1299

Closed fyc09 closed 1 month ago

fyc09 commented 1 month ago

Fix #1268.

Side Effect: MD013 will treat html code (including embedded markdownlint-configure-file) as code blocks, which will emit errors in these cases:

https://github.com/DavidAnson/markdownlint/blob/b2305efafb034b1f328845aec9928b5363ffd646/test/long-lines-short-code.md?plain=1#L27-L32 https://github.com/DavidAnson/markdownlint/blob/b2305efafb034b1f328845aec9928b5363ffd646/test/long-lines-short-code.md?plain=1#L27-L32 https://github.com/DavidAnson/markdownlint/blob/b2305efafb034b1f328845aec9928b5363ffd646/test/long-lines-strict.md?plain=1#L33-L39

DavidAnson commented 1 month ago

"Code" in the case of getLineMetadata is meant to refer to one of the two Markdown code block types (indented or fenced). The proposed change could have more side effects beyond what you have already identified. I had been thinking that a fix specific to MD018 would be best here. Did you try that first?

DavidAnson commented 1 month ago

Thank you! This looks more like what I was imagining on first glance. Could you please re-base this PR to be for next branch? (See CONTRIBUTING.md) FUI, if there are conflicts in the generated file, you can ignore thwm and then regenerate that file. I'll approve the PR checks then and do a proper review.

DavidAnson commented 1 month ago

By the way, could you please update the test file to include scenarios that would trigger rules 18, 19, 20, and 21? If 18 is broken, then I suspect 20 is also. 19 and 21 are already migrated to micromark in the next branch, so should be OK as-is. But I would like the new test file to verify the HTML scenario for all four of these similar rules. Thank you!

fyc09 commented 1 month ago

I'm unsure if we should investigate this violation, as the current implementation will simply ignore it.

##Title <span>text</span> {MD018} {MD033}
DavidAnson commented 1 month ago

What you show above seems like correct behavior, or am I missing something?

DavidAnson commented 1 month ago

FYI, there will be a failure in TeatRepos because that is failing in next branch. I will fix that shortly, so please ignore for now.

fyc09 commented 1 month ago

What you show above seems like correct behavior, or am I missing something?

Currently, only MD033 is emitted; MD018 is ignored due to the HTML block in this line. I think emitting both MD033 and MD018 will be more reasonable.

DavidAnson commented 1 month ago

I agree. But the above is an HTML span, not a block. I would not expect it to be affected by the new code?

fyc09 commented 1 month ago

I agree. But the above is an HTML span, not a block. I would not expect it to be affected by the new code?

You are right, I made an mistake.

DavidAnson commented 1 month ago

Please rebase PR to latest next branch commit to fix "sh: 1: ava: not found". This is a bug in latest Node/npm which I have worked around.

DavidAnson commented 1 month ago

I can't easily view the snapshot diff on my phone, but all the other code changes look good, thank you!

DavidAnson commented 1 month ago

I had a look at the differences, and there is way more going on than there needs to be. After you re-base (see comment above), please run "npm run update-snapshots" to regenerate that file. The diff at that point should be ONLY the errors and corrections for the new test file you added. After that, we make sure checks pass and I'll approve. :)

fyc09 commented 1 month ago

I reran "npm run update-snapshots", but the snapshot didn't change, and I enabled GitHub action in my own repository and the CI looks good. I think the tests' order is changed, but I do not know what to do.

DavidAnson commented 1 month ago

I wonder if this is a locale issue that sorts differently for you? I will give it a try in a day or two and either push a commit to your branch with the "right" order or approve your changes as-is. Thanks again!