E3SM-Project / E3SM

Energy Exascale Earth System Model source code. NOTE: use "maint" branches for your work. Head of master is not validated.
https://docs.e3sm.org/E3SM
Other
332 stars 334 forks source link

use a markdown linter on new files #6342

Closed mahf708 closed 1 month ago

mahf708 commented 1 month ago

Adds a markdown linter on new files in PRs to ensure they conform with some linting rules (can modify these later if needed).

[BFB]

github-actions[bot] commented 1 month ago

PR Preview Action v1.4.7 :---: :rocket: Deployed preview to https://E3SM-Project.github.io/E3SM/pr-preview/pr-6342/ on branch gh-pages at 2024-04-15 21:33 UTC

rljacob commented 1 month ago

The linter will only run on new or modified files?

mahf708 commented 1 month ago

Where will the linter report? Do we just open the action log and see some output from it?

The action is one thing that happens, yes, but this action actually writes on the PR too! I can show you an example

mahf708 commented 1 month ago

The linter will only run on new or modified files?

Yes. The file has to be touched somehow in the PR for this action to run on it.

The reason I chose this: I didn't want to open a can of worms about fixing older files now. There will be numerous small fixes, which I can take care of if there's appetite (I can do so automatically, essentially, so I don't actually mind doing it, I just didn't want to insert myself into older files). If you want me to sweep through all .md files while at it, I can do that in this PR, but I will likely end up editing every single .md file (minor formatting edits).

rljacob commented 1 month ago

Good I want it to just look at new/modified files. Don't fix any old ones.

mahf708 commented 1 month ago

@rljacob let's hold off merging until I investigate how strict this linter is (and how to customize it further). I don't want it to be too strict (e.g., in https://github.com/E3SM-Project/E3SM-Project.github.io/pull/2 it is complaining about line length, which may be a good idea in regular code, but may be too burdensome for markdown...)

mahf708 commented 1 month ago

Should be good to go. This will give people trouble and will force them to format their docs better, and I think that's a net positive for everyone. I will add some instructions on how to preemptively format docs to avoid violating these rules once we migrate the "develop docs" guide

rljacob commented 1 month ago

How do I run this in my docs development environment on my laptop?

mahf708 commented 1 month ago

How do I run this in my docs development environment on my laptop?

A few options from preferred to less so:

My thinking was: adding this linter will be helpful in reviewing new docs, and so my hope is this would just constitute live feedback in the PR process, and users won't have to deal with it locally. Otherwise, it gets a little too complex very quickly

mahf708 commented 1 month ago

An alternative: we can use pre-commit which can also actually fix the lints for the user

rljacob commented 1 month ago

pre-commit hook would be good. Can you add one to https://github.com/E3SM-Project/E3SM-Hooks ?

mahf708 commented 1 month ago

Ok, will do. Let me test it before we merge. Going back to draft while I do that 😄

rljacob commented 1 month ago

Actually the hooks in that repo are bash-based hooks that run anywhere. It looks like the pre-commit hook for the linter needs python and some things installed.

mahf708 commented 1 month ago

Yes, so do you just want people to get review on linting in PRs? That's what I had in my head when I added this. This specific action added here does give very nice annotations on exact lines of code in the "file" view of the PR, so it is like a human commenting... I can still look into a pre-commit-like solution. I just need to find a moment to do that (and experiment locally)

(I guess my philosophy and open-source "upbringing" assume that people won't do much/anything locally --- so, giving them all the tools to submit interactive PRs or web-based PRs when appropriate; in fact, if you look closely, this exact PR by me is 100% web-based, hence, the "Verified" tags...)

bartgol commented 1 month ago

This action already blocks the PR merging, right? If so, I'm ok if with an action (rather than a hook), forcing the dev to do a bit of work.

Quick question for @mahf708 : does this action only run on new files? What if someone modifies an existing one, using bad formatting?

mahf708 commented 1 month ago

Quick question for @mahf708 : does this action only run on new files? What if someone modifies an existing one, using bad formatting?

It runs on changed files, which includes new and modified. Looking at it in https://github.com/E3SM-Project/E3SM-Project.github.io/pull/2, it does run on both .md files changed in that PR, which includes a new one and an old one (that's modified).

    - uses: tj-actions/changed-files@v44
      id: changed-files
      with:
        files: '**/*.md'
        separator: ","
mahf708 commented 1 month ago

Just for completeness: I have tested a pre-commit approach that works well. I am not adding it to this PR because I am afraid it will cause more problems than we want. Specifically, I am worried that users will start hitting it and complain that it is giving them a hard time. In the future, I will add all these details to the guide for developing docs as well as thinking about a solution (I will try to add this linter to conda-forge as well). A lot of words, but I think we do have this sufficiently in order, and we can go ahead and merge. We can always revert if needed. I am always available to help users with issues in PR (a tag away!)