update CI (again)

[2bndy5]

Opening this to discuss changes to the CI workflow...

• [x] Run Py and JS linting/format checks in a single job (agnostic of OS). OS specific checks (like unit tests and docs build) will require this job to complete first.
• [x] Use nox to pin certain versions of dev tools (namely the python tools like black, mypy, etc). ~There might be a way to incorporate the npm run check step with the pre-commit run command, but I'm not sure if this is preferable/possible.~ EDIT npm run check will still be used separately from pre-commit.
• [x] Generate code coverage reports from unit tests (this may be optional but informative)
• [x] Avoid duplicate CI triggers in open PRs (using suggestion from https://github.com/jbms/sphinx-immaterial/pull/186#issuecomment-1304636511)

The main motivation here is to reduce the workflow run time by implementing task 1. Contributions from #220 and #219 will be retained.

[jbms]

I think it would definitely be useful to have a script to run to execute all of the lint/format checks. I suppose right now we have some checks via npm run commands but the Python checks are not included there. We could pin the versions in dev-requirements.txt

I'm not familiar with the pre-commit python package. In general I don't use actual git pre-commit hooks myself --- with the way I work with git, I want creating a commit to be a lightweight operation and not have it run tests as part of it. I may often initially commit a work in progress and then amend/rebase later to fix it.

Code coverage and avoiding duplicate CI triggers sounds good.

As far as waiting until the lint checks pass to run the OS specific tests --- at the moment there doesn't seem to be an issue with the amount of github actions usage, though I suppose it might become an issue in the future. Sometimes it is annoying to skip other tests just due to formatting errors.

[2bndy5]

We could pin the versions in dev-requirements.txt

This is better maintained via pre-commit config. But, if you're not going to use pre-commit locally (which will still work fine), then we'd have to specify the dev tools' versions in both dev-requirements.txt and .pre-commit-config.yaml

As far as waiting until the lint checks pass to run the OS specific tests --- at the moment there doesn't seem to be an issue with the amount of github actions usage, though I suppose it might become an issue in the future.

With #220 merged, it would be more convenient to have a single job prevent entering a matrix. Otherwise, each job in the matrix could fail independently and unanimously because the beginning steps (lint/format checks) are all identical.

Sometimes it is annoying to skip other tests just due to formatting errors.

This is a good point, but I prefer the CI workflow be as strict for push events as would be expected for release events.

---

BTW, the fourth point (referring to @mhostetter suggestion) would limit triggers to PRs targetting main and any push event on main. So, a push event on a dev branch (without an open PR to main) wouldn't incur CI runs. At least that's how I understand it.

PS - We're already at 66% coverage 🎉 IDK if you're keen to uploading coverage reports to codecov.io (which would probably require you're authorization), but if not, I can just add a CI summary of the coverage results instead using GITHUB_STEP_SUMMARY.

[2bndy5]

I forgot to mention that pre-commit can be used to programmatically check for things that can be easily missed like

• force LF usage in all files
• ensure files end in a blank line
• validate syntax in yaml, json, and requirements.txt files
• prevent large files being committed (say anything over 1000 kB)

All of these checks (AKA "pre-commit hooks") can be configured to narrow the scope from the entire repo to certain paths. For instance, I don't want pre-commit to focus on anything inherited from upstream (npm run check is adequate to handle that stuff)