Skip to content

Improve Markdown linting and use it on more files #4515

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

Merged
merged 8 commits into from
Apr 24, 2025

Conversation

lornajane
Copy link
Contributor

Fixes #4249 which asks for Markdown linting to be applied to the markdown files at the root level of the repo.

Points to look for:

  • I added a second markdown lint configuration, I expect the standards for the spec and for our everyday markdown files to diverge so while they're not all that different now, I still think it's worth the overhead.
  • We no longer use mdv, it is outdated and we can do most of what's needed with markdownlint. My only concern is that we're missing some formatting of json/yaml/abnf code examples, so we should consider any options on that (and be careful with them in the meantime!)
  • I also removed prettier. We had a script which ran prettier and then did a sed manouvre to fix it. I don't think the benefits of using prettier are worth having this sort of hackery in the project so we'll use the markdownlint "fix" feature instead and I removed the script.

I realise that src/oas.md isn't present on this branch but the same configuration seemed to be okay with what's currently on both v3.1-dev and v3.2-dev.

  • schema changes are included in this pull request
  • schema changes are needed for this pull request but not done yet
  • no schema changes are needed for this pull request

Sorry, something went wrong.

@lornajane lornajane requested review from a team as code owners March 28, 2025 16:39
@ralfhandl
Copy link
Contributor

The validate-markdown.yaml workflow still calls mdv - should that call be removed?

It also calls markdownlint-cli --config .markdownlint.yaml on the spec files:

  • old cli instead of cli2
  • uses config file that is now intended for non-specs on the spec files

@ralfhandl ralfhandl requested a review from a team March 28, 2025 22:11
Copy link
Contributor

@ralfhandl ralfhandl left a comment

Choose a reason for hiding this comment

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

Good direction, let's just remove mdv everywhere.

@handrews handrews added the script Pull requests that update Bash or JavaScript code label Apr 11, 2025
@lornajane
Copy link
Contributor Author

Good catch on the dependencies! Using npx doesn't need the packages pre-installed so they don't need to be in the dependencies list.

@lornajane lornajane requested a review from ralfhandl April 15, 2025 18:31
@lornajane lornajane force-pushed the better-markdown-lint branch from 18d0f0a to d301477 Compare April 15, 2025 18:36
@lornajane
Copy link
Contributor Author

Hmm. Those GitHub workflows seem to check the old versions (which should be immutable!), this work is to improve the standards for the in-progress spec versions on the two dev branches, and all *.md files at the root of the project in all branches. Although now I've removed the libraries from the dependencies, maybe we should do something with those as well. At least for the ones that are 3.0.4 or 3.1.1 and later since they should be at a standard that works. Or stop validating the version files because everything shoud have been in a valid src/oas.md anyway before it got baked into a release?

use markdownlint-cli2 and refresh package-lock.json
@ralfhandl
Copy link
Contributor

stop validating the version files because everything shoud have been in a valid src/oas.md anyway before it got baked into a release?

That would be my way forward. I've sketched what I'd do in

@lornajane The above mentioned PR is in your repo and will update this PR's head branch. Feel free to review, merge (in your repo), do something similar in your head branch, ... 😎

@lornajane
Copy link
Contributor Author

Thanks @ralfhandl , I merged your changes (with a minor edit so we only validate and don't fix in the workflow), could you take another look please?

@ralfhandl ralfhandl requested a review from a team April 22, 2025 07:25
@ralfhandl
Copy link
Contributor

Note: changed branch protection rule for main and made the lint job a required status check instead of its old name mdv.

The same change will be needed for other branch protection rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
script Pull requests that update Bash or JavaScript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

validate-markdown: include markdown files in project root
3 participants