Code style additions #3651
Code style additions #3651
Conversation
AoifeHughes
added
enhancement
✅ Deploy Preview for the-turing-way ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Actually this is kinda fun, adding the pre-commit file has flagged https://github.com/the-turing-way/the-turing-way/actions/runs/9063649234/job/24900034368?pr=3651 which I don't get why the spaces around hyphen thing is an issue here |
|
Latest commit should be CRefed with #3650 (comment) |
|
To make it easier for a reviewer, I think the only file that really needs checked over is: |
|
@JimMadge would this be something you've capacity for to give a quick look over? I've already requested a review from @aleesteele too. |
There was a problem hiding this comment.
Looks great 🎉.
A few suggestions for the text and a question about the changed images.
I think the hooks you've added are uncontroversial. As long as CI will apply the changes, and we don't "force" users to add pre-commit to their workflow I think it will be a great addition for code quality 🙌.
.pre-commit-config.yaml
Outdated
There was a problem hiding this comment.
We could also use Prettier for format Markdown/YAML/JSON.
I've had good experiences using it for websites written in Markdown before.
That might be a good thing to do in another PR though as it will likely make a lot of change (and might break the rendering of a few things).
book/website/figures/README.gif
Outdated
There was a problem hiding this comment.
Not sure what has happened here. Has the image been removed?
There was a problem hiding this comment.
No, it's still here
but it does seem broken? Lemme see if I can fix it
There was a problem hiding this comment.
Interesting that whitespace was tidied in svgs 😄.
It might be confusing, maybe we should exclude svgs from linting?
book/website/reproducible-research/code-quality/code-quality-style.md
Outdated
Show resolved
Hide resolved
|
|
||
| - Runs checks before each commit | ||
| - Supports a wide range of programming languages | ||
| - Configurable through a `.pre-commit-config.yaml` file in your repository |
There was a problem hiding this comment.
(My opinion here)
It might be worth saying something about how pre-commit helps teams work together.
Git hooks have existed for ages, but the things pre-commit adds are,
- Easily sharing hooks in the repo
- Running hooks on the user side before commit/push instead of on the remote side
| - Supports a wide range of programming languages | ||
| - Configurable through a `.pre-commit-config.yaml` file in your repository | ||
| - Extensible with custom hooks | ||
| - Integrates with version control systems like Git |
There was a problem hiding this comment.
Links to the Glossary for VCS and Git would be good.
|
|
||
| To use pre-commit: | ||
|
|
||
| 1. Install pre-commit: `pip install pre-commit` |
There was a problem hiding this comment.
I would be tempted to link to pre-commits own install docs here.
(That and I don't like pip install outside of a venv 😄)
book/website/reproducible-research/code-quality/code-quality-style.md
Outdated
Show resolved
Hide resolved
book/website/reproducible-research/code-quality/code-quality-style.md
Outdated
Show resolved
Hide resolved
| ```yaml | ||
| repos: | ||
| - repo: https://github.com/pre-commit/pre-commit-hooks | ||
| rev: v4.0.1 | ||
| hooks: | ||
| - id: check-yaml | ||
| - id: end-of-file-fixer | ||
| - id: trailing-whitespace | ||
| ``` | ||
|
|
||
| This configuration uses the `pre-commit-hooks` package for general-purpose checks and the black package for Python code formatting. |
There was a problem hiding this comment.
I think the file doesn't match the description? There isn't a hook for Black.
…tyle.md Co-authored-by: Jim Madge <jmadge@turing.ac.uk>
|
TODO:
|
…tyle.md Co-authored-by: Jim Madge <jmadge@turing.ac.uk>
…tyle.md Co-authored-by: Jim Madge <jmadge@turing.ac.uk>
Summary
Addresses #3650 and also adds information following on from #1390
This PR adds a new section on pre-commit hooks to the "Code Style and Formatting" chapter.
It is also important to decide if the
.pre-commitfile added should be ran. If so then can we do it here (~500 files will be changed by having line-endings etc. corrected)List of changes proposed in this PR (pull-request)
What should a reviewer concentrate their feedback on?
Acknowledging contributors