pre-commit-related updates#1345
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes small maintenance updates to the repository’s pre-commit setup and developer documentation, including removing an unused hook, updating a hook source URL, and documenting an alternative tooling option.
Changes:
- Remove the
nbstripoutpre-commit hook from.pre-commit-config.yaml. - Update the
cffconverthook repository URL. - Update docs to mention
prekas apre-commitalternative and fix a small glossary typo.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
docs/glossary.md |
Fixes formatting/typo in the “Commodity” definition. |
docs/developer_guide/setup.md |
Documents prek as an alternative and adds the reference link. |
.pre-commit-config.yaml |
Removes nbstripout and updates the cffconvert repo URL. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1345 +/- ##
=======================================
Coverage 89.54% 89.54%
=======================================
Files 58 58
Lines 8525 8525
Branches 8525 8525
=======================================
Hits 7634 7634
Misses 580 580
Partials 311 311 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| args: [--ignore-words, .codespell_ignore.txt] | ||
| exclude: \.svg$ | ||
| - repo: https://github.com/EnergySystemsModellingLab/cffconvert | ||
| - repo: https://github.com/ImperialCollegeLondon/cffconvert |
There was a problem hiding this comment.
Why do we need to use a fork?
There was a problem hiding this comment.
It's a really annoying issue where there isn't a release for cffconvert which includes the pre-commit hooks. You can tell pre-commit to use a particular commit rather than a release, but pre-commit autoupdate fails in this case, preventing us from updating any of the pre-commit hooks. Should probably open an issue for pre-commit for it.
The fork doesn't have any new code, just a new release.
There was a problem hiding this comment.
...and I've now moved the fork to the ICL repo because we're using it for another project.
| commit is made. You can run `pre-commit` via our `justfile`, provided you have `uv` installed. | ||
|
|
||
| Alternatively, you can use [prek], which aims to be a drop-in replacement for `pre-commit`. Being | ||
| written in Rust, it has no dependencies and can be installed with `cargo`. |
There was a problem hiding this comment.
Interesting. Didn't know this existed. Wonder if it might be worth making this the default instruction, rather than two alternatives which might be confusing.
Could this have cleared up some of the issues Melissa was having with pre-commit, for example?
There was a problem hiding this comment.
Yeah, I'm wondering that too. I've been using it for all my projects for months now instead of pre-commit, without issue. It's probably easier to set up given that developers will have cargo anyway.
Melissa was actually able to get things working with prek in the end (we gave up on pre-commit).
Description
A few small
pre-commit-related changes:nbstripouthookcffconverthookI toyed with doing #582 and also adding a hook for mdformat while I was at it, but the alternative YAML formatter ended up fighting with another hook and mdformat was making more changes to the md files than I was comfortable with (not sure if mdbook's slightly weird equation handling might be broken by it), so I left these for now.
Unrelated change:
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks