Skip to content

fix: MD022 space around headers#15310

Merged
mairaw merged 2 commits intodotnet:masterfrom
nschonni:fix--MD022-space-around-headers
Nov 10, 2019
Merged

fix: MD022 space around headers#15310
mairaw merged 2 commits intodotnet:masterfrom
nschonni:fix--MD022-space-around-headers

Conversation

@nschonni
Copy link
Copy Markdown
Contributor

@nschonni nschonni commented Oct 20, 2019

Summary

Trying out the experimental --fix flag on the VB docs after enabling the MD022 rule. Actual diff was 2K documents for that folder so just added a small subset.

Fixes #Issue_Number (if available)

Using experimental --fix flag
Copy link
Copy Markdown
Contributor

@mairaw mairaw left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @nschonni.

Copy link
Copy Markdown
Contributor

@mairaw mairaw left a comment

Choose a reason for hiding this comment

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

Argh, just noticed there are some merge conflicts. Can you fix those? Thanks.

@nschonni
Copy link
Copy Markdown
Contributor Author

@mairaw I can fix this by re-running it, but do you want to fix the (whole) repo yourself now that the --fix option has landed in the published release of https://github.com/igorshubovych/markdownlint-cli

  1. Run npm install -g markdownlint-cli to install the latest CLI
  2. Remove the "MD022" line from the .markdownlint.json
  3. Run markdownlint --fix "**/*.md"
  4. Profit 💰

@mairaw
Copy link
Copy Markdown
Contributor

mairaw commented Oct 22, 2019

Neat idea but I'm a bit on the fence if I really wanna do this global change like this. @dotnet/docs any thoughts?

@nschonni
Copy link
Copy Markdown
Contributor Author

You can do it in pieces by changing the glob pattern too. EX: markdownlint --fix "doc/visual-basic/**/*.md" fave ~2K changes, and this PR was just staging and committing the first 100 files

@mairaw
Copy link
Copy Markdown
Contributor

mairaw commented Oct 22, 2019

I get that and I usually submit global changes piecemeal. My point is more: I'll gladly review and accept PRs for this kind of change, but this is not a high priority for our team to go do it at this moment, given the size of our backlog. I hope it makes sense.

@tdykstra
Copy link
Copy Markdown
Contributor

Neat idea but I'm a bit on the fence if I really wanna do this global change like this. @dotnet/docs any thoughts?

@mairaw I prefer not to do bulk changes to implement rules that don't have any negative impact on doc rendering. In this case I would say the rule itself is unnecessary. I follow the pattern dictated by the rule myself but I don't see the need to impose it on everyone. @nschonni I appreciate your time spent standardizing our Markdown, I just am not convinced of the benefit of this one.

Regarding bulk updates in general, I would do them only where it's demonstrably necessary. I've seen instances where they've been done without carefully reviewing each and every change, and in some contexts the change doesn't make sense or the author had a reason for doing things a particular way, a reason not immediately apparent to a superficial skimming of changes.

@nschonni
Copy link
Copy Markdown
Contributor Author

@tdykstra this rule does affect rendering on GitHub, but AFAIK, docfx didn't tighten its rendering at the same time a year ago

@Thraka
Copy link
Copy Markdown
Contributor

Thraka commented Oct 22, 2019

@nschonni Can you show an example; screenshot perhaps? I don't see any difference in rendering. :)

@nschonni
Copy link
Copy Markdown
Contributor Author

nschonni commented Oct 22, 2019

Sorry, I think I'm mixing up the rule where the hash is stuck at the beginning of the heading. EX:

##Not rendered as heading

VS Code and some others don't like the stuck content in some cases for syntax highlighting.

<a href="dumy">
## When to Create a Component  

@mairaw
Copy link
Copy Markdown
Contributor

mairaw commented Oct 23, 2019

I think we've started adding them as part of the heading title. Let me see if I can find an example.

Copy link
Copy Markdown
Contributor

@mairaw mairaw left a comment

Choose a reason for hiding this comment

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

Thanks @nschonni!

@mairaw mairaw merged commit b5b4393 into dotnet:master Nov 10, 2019
@nschonni nschonni deleted the fix--MD022-space-around-headers branch November 10, 2019 04:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants