cudf
cudf copied to clipboard
lint: use `mdformat` for sanitizing MD files
Description
a minor suggestion to keep all MD files sanitized and validated by pre-commit hook
Checklist
- [x] I am familiar with the Contributing Guidelines.
- [x] New or existing tests cover these changes.
- [x] The documentation is up to date with these changes.
This pull request requires additional validation before any workflows can run on NVIDIA's runners.
Pull request vetters can view their responsibilities here.
Contributors can view more details about this message here.
@Borda Is there something specific that you want to see changed? It would be helpful to share more of the motivation for this PR.
I see you added the pre-commit hook but did not apply it. I looked over some of the changes and I think we would need to add exclusions for .github/ and possibly other paths (or rewrite our files in a different way), because it's inserting escape characters in places where the text will appear literally rather than being rendered.
There are some helpful formatting changes like adding line breaks after section headers, but I don't agree with all of the choices this linter makes. For example, forcing all numbered lists to be 1. and relying on Markdown renderers to count upward. We tend to prefer human readability of the raw Markdown. (edit: it looks like that is configurable with --number, maybe we just disable it)
Is there something specific that you want to see changed?
in fact yes, after running this hook it generates lots of edits so I was not sure if I should commit them or do you prefer another way to get them in...
It would be helpful to share more of the motivation for this PR.
I was about send some small fix to the readme but then I start thinking it would be better to have a check that the edits are correct so pivoted just to adding the check intead
I looked over some of the changes and I think we would need to add exclusions for
.github/and possibly other paths (or rewrite our files in a different way), because it's inserting escape characters in places where the text will appear literally rather than being rendered.
yes, so shall I apply changes and you may have look and comment on changes that shall not be applied - likely being skipped?
forcing all numbered lists to be
1.and relying on Markdown renderers to count upward. We tend to prefer human readability of the raw Markdown. (edit: it looks like that is configurable with--number, maybe we just disable it)
yes that is a good point :)
/okay to test
I'm not sure I agree with all the changes here. For example, the adding and removing of whitespace. How much control do we have over the format settings? Are they listed somewhere? I don't see a config file like for other linters. Is this a problem that needs to be solved? I'd like to see some other opinions. I'm also concerned that these files are consumed by the doxygen and sphinx tools and may ultimately render incorrectly with the new formatting. I would be happy with a linter that ensures these tools work well.
I don't see a config file like for other linters.
I do not see many other options for the numbers and wrap size - https://mdformat.readthedocs.io/en/stable/users/installation_and_usage.html#id1
How much control do we have over the format settings? Are they listed somewhere?
you can control the used version so I would say it the same as Black, you use it or don't :)
@davidwendt @bdice WDYT since you two reviewed here? It sounds like the changes that this linter makes may not be what we want, or at least not quite configurable enough, IIUC. Are there some edits in this PR that we would maybe like to keep without enabling the linter?
...Are there some edits in this PR that we would maybe like to keep without enabling the linter?
That is a good question. I read through all the cpp/* changes and did not see anything that was worth keeping. Actually some of the changes to the regex.md are invalid -- I did not notice these before.
I briefly scanned the other files and also did not notice anything compelling in those files either.
@Borda thanks for the PR! Based on the discussion above I'm going to close this as it doesn't look we want to add this linter, but we do appreciate the contribution! Please do open future PRs (or issues to propose improvements) if you have ideas!