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

Markdownlint CHANGES.md #12431

Merged
merged 7 commits into from
Jan 14, 2025
Merged

Markdownlint CHANGES.md #12431

merged 7 commits into from
Jan 14, 2025

Conversation

jjspace
Copy link
Contributor

@jjspace jjspace commented Jan 10, 2025

Description

We were ignoring the CHANGES.md file from linting. I assume this is just because there were so many errors in there. Instead of fixing them all I disabled the 3 that were causing the most issues:

  • MD001/heading-increment: Heading levels should only increment by one level at a time
  • MD031/blanks-around-fences: Fenced code blocks should be surrounded by blank lines
    • This can be annoying specifically in bulleted lists which is this entire file, just ignore it here
  • MD046/code-block-style: Code block style should be fenced
    • All of the older changes used indented code blocks which are worse and can't have syntax highlighting but I opted to just disable this for the second half of the file to leave them as is

I also did a spelling pass on this file. I skipped acronyms because I'm not sure the best approach to those yet but the other changes are all valid.

Issue number and link

no issue

Testing plan

Make sure npm run markdownlint doesn't return any errors

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @jjspace!

✅ We can confirm we have a CLA on file for you.

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Thanks @jjspace! Yes, we skipped linting this file when first implementing markdownlint because of a good majority of it is non-compliant.

  1. Any reason you can think of to not fix violations of MD001/heading-increment? While we do require specific (lack of) formatting for release notes to be picked up on the website, I don't think any of our automated processes rely on the CHANGES.md structure. I believe this can be fixed automatically by passing --fix option to markdownlint-cli.

  2. I'd like to avoid being pick-and-choosy about applying linting rules like MD031/blanks-around-fences. I think we should either disable it everywhere or nowhere. I also believe violations of this rule can be fixed automatically by passing --fix option to markdownlint-cli.

I agree that going back and fixing violations of MD046/code-block-style are likely moot since they will require some manual input and are just old at this point.

@jjspace
Copy link
Contributor Author

jjspace commented Jan 13, 2025

Any reason you can think of to not fix violations of MD001/heading-increment

This would change almost every single heading in the file because it's "wrong" right at the top where we go from H1 to H3.

# Changes

### 1.123 Version

I just didn't think it was worth going through them all 1 by 1. This is not a --fixable rule because there's no easy way to automate decisions around this. Either you decrement the level of all headers or you simply add the missing level once.
Especially hard (to automate) when the "errors" start to "layer" like would be needed halfway through the file where we have. Does H1 > H3 > H5 go to H1 > H2 > H3 (like I'd do for this case) or H1 > H2 (new) > H3 > H4 (new) > H5 or something in-between.

### 1.88 - 2021-12-01

##### Fixes :wrench:

The other reason I left it alone is because Github makes a distinction that H1 and H2 get a horizontal line break and H3 onward don't. This would change the way our CHANGES actually look (maybe for the better) but worth noting.

Headings

H1

H2

H3

I'm still willing to go through and do these in the interest of correctness, it shouldn't take that long but let me know what you think.

I'd like to avoid being pick-and-choosy about applying linting rules like MD031/blanks-around-fences

I also would like to avoid picking and choosing. The motivation here was because this file is nearly entirely lists and extra lines between items in a list make them render much more spread out. I didn't realize the rule had a special override for these code blocks. Check the dropdown for a demonstration of the difference but I've opted to turn off the blank line rule for list items as I feel the tighter lists look much better

Code blocks in lists

No Blank line:

  1. item 1
  2. item 2
    // code block
  3. item 3
  4. item 4

Blank lines only around code block:

  1. item 1

  2. item 2

    // code block
  3. item 3

  4. item 4

Notice that it forces extra space around every list item even if they are not next to the code block

@ggetz
Copy link
Contributor

ggetz commented Jan 14, 2025

Thanks @jjspace.

MD001/heading-increment

Ah, I was hoping the --fix option would raise the h3 option to an h2 and so on. I don't have much of an opinion on the formatting. I'd rather conform to standards for the sake of accessibility.

IMO we should do the fix, but I'm not going to hold up this PR over it. Just let me know your plan @jjspace.

MD031/blanks-around-fences

I agree and I'm happy with this config 👍

@jjspace
Copy link
Contributor Author

jjspace commented Jan 14, 2025

MD001/heading-increment

@ggetz I've corrected all heading nesting. It actually went faster than I expected using some search and replace in VSCode. It does make quite a big file diff though but hopefully still easy to review.

I think this should be good to go now

@ggetz
Copy link
Contributor

ggetz commented Jan 14, 2025

Fantastic, thanks @jjspace!

@ggetz ggetz enabled auto-merge January 14, 2025 21:31
@ggetz ggetz added this pull request to the merge queue Jan 14, 2025
Merged via the queue into main with commit dc667cd Jan 14, 2025
9 checks passed
@ggetz ggetz deleted the markdownlint-changes branch January 14, 2025 21:31
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.

2 participants