cudf icon indicating copy to clipboard operation
cudf copied to clipboard

lint: use `mdformat` for sanitizing MD files

Open Borda opened this issue 1 year ago • 7 comments

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.

Borda avatar Apr 06 '24 20:04 Borda

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.

copy-pr-bot[bot] avatar Apr 06 '24 20:04 copy-pr-bot[bot]

@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)

bdice avatar Apr 08 '24 13:04 bdice

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 :)

Borda avatar Apr 08 '24 13:04 Borda

/okay to test

galipremsagar avatar Apr 09 '24 15:04 galipremsagar

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.

davidwendt avatar Apr 09 '24 20:04 davidwendt

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 :)

Borda avatar Apr 12 '24 20:04 Borda

@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?

vyasr avatar May 21 '24 18:05 vyasr

...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.

davidwendt avatar May 31 '24 13:05 davidwendt

@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!

vyasr avatar Jun 03 '24 16:06 vyasr