-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
fix: MD031/blanks-around-fences #2458
fix: MD031/blanks-around-fences #2458
Conversation
LGTM. I noticed upstream they have another way of linting Markdown files. Perhaps here the rules should be similar so that there's no need to adapt to yet another code style. |
Upstream? |
I mean node core. Don't get me wrong, I like these changes for sure. I just think the ultimate goal should be to have a config that's safe and also is close to upstream so that people coming from core to contribute here don't need to adhere to a completely different style :) |
Core uses remark-lint and publishes the settings at https://github.com/nodejs/remark-preset-lint-node so that other projects can easily import/use them. |
@Trott i'll take a look at swapping Markdownlint for that then. Not sure if this still makes sense for the remark rules as i'm not familiar with them |
OK, I've opened up #2460 for swapping to the preset. Just disabled the failing rules for now |
Not sure, I'm not really familiar with remark-lint, but none of the ones I had to disable sounded similar https://github.com/nodejs/nodejs.org/pull/2460/files#diff-cf49712b540c90b3c7b5d2ed691a2cacR4-R24 |
We'll see in the PR switch. From a quick look remark-lint has a lot more different rules. |
This looks like it should be |
Because it's not enabled in the node preset :) |
Yup, can always see if they want to enable that after |
I tried enabling it in core and it results in a lot of warnings, but I'm OK with adding it. Might take some time and others might object to the churn. We'll see.... |
Sounds good, could also just enable it on top of the shared config in this repo if the other PR lands |
You might want to do that permanently. Looking a bit more closely, this flags some things in core that might be hard-sells to enforce. They have an option to disable it for nested lists, which is good. But it doesn't look like there's any way configuration-only way to disable it flagging things like: ### Some header
<a name="some-name"></a> ...or preceding a code fence with an HTML comment to disable code linting. This will require a blank line in those instances and a case can be made that putting a gap between those things separates things that really should be together. But it wouldn't surprise me if those things don't exist in the website code base. (And if they do, then making it work over here might be a good first step before trying to bring it into core.) A longer road might be to PR in some configuration options into remark-lint-no-missing-blank-lines. And an even bigger idea might be to move Node.js core to markdownlint. |
@Trott is that a TSC type thing for swapping something like that in the org? |
TSC only gets involved if Collaborators can't come to an agreement on something. It's more of a "someone opens a pull request, maybe tries to make the case for why the change is an improvement, and people discuss" thing. |
I think the best solution would be if we switched to remark-lint here and we adapt the node preset to fit our needs here. It's OK if the config isn't 100% identical. For example, here we don't need the anchor comment before the header; the comment needs to be in the header itself. |
cc029a2
to
9a358f8
Compare
Fenced code blocks should be surrounded by blank lines. Mostly find/replace for triple backtick after colon
9a358f8
to
f52650e
Compare
Fenced code blocks should be surrounded by blank lines.
Mostly find/replace for triple backtick after colon