RPi-Jukebox-RFID icon indicating copy to clipboard operation
RPi-Jukebox-RFID copied to clipboard

🚀 | Devtools: use pre-commit with more hooks

Open votti opened this issue 2 years ago • 4 comments

Feature Description

Currently there are only few pre-commit hook setup with a custom bash script.

The excellent pre-commit framework could be used to enforce more code standards in an automatized, standardized, easy-to-use manner.

How do you envision the feature to work from a users perspective?

In order to run the pre commits from pre-commit, the pre-commit python package needs to be installed: https://pre-commit.com/index.html#install

The .pre-commit-config.yaml file could be checked into the repository, containing all the hooks that should be enforced.

To make the transition easy, maybe first the new.pre-commits could be not enforced as part of cicd but just added as a suggestion

Further information that might help

I have set this up on my fork: https://github.com/votti/RPi-Jukebox-RFID/tree/future3/precommit Therin I have setup a couple of hooks, most notably:

  • black: automated code formatting
  • mypy: checking code annotations
  • pep8: checks pep8

if there is interest, I am happy to write a proper pull request.

votti avatar Dec 30 '22 13:12 votti

From my point of view this is a good suggestion.

@pabera @ChisSoc

s-martin avatar Dec 30 '22 14:12 s-martin

Sounds interesting. Currently we are running flake8 and doing a trial build of the docs. Both have a project-specific configuration.

When switching to pre-commit, I'd expect these two checks to be run with the same properties as before.

For black, I'd leave it a suggestion for now. Generally, formatting in the code base is not that bad. Still, running black on the present files, will likely change every file. That should be a well timed commit and separate from everything else.

I am not sure the code base is ready for myst. I was struggling a bit with it for the plugs module. Have you run myst? If we make that a pre-commit hook, I do not want lots of warnings popping up from the current code base and confusing new developers.

ghost avatar Jan 01 '23 11:01 ghost

It definitely could make sense to make black and mypy fix the whole repo before switching the hooks on. This includes some one time efforts and only makes sense once there is a decision that pre-commit with these checks makes sense long term and there is a concrete commitment to switch to it.

Personally I have not run into issues with mypy. But then the pre-commits only run for files I changes, so maybe I did not touch the problematic files yet :)

Keeping the old checks working as-is would be easy. Flake8 has official support and sphinx has an unofficial plugin: https://github.com/thclark/pre-commit-sphinx

votti avatar Jan 04 '23 21:01 votti

As a developer, I'd like to know how to enable these helpers in the IDE, specifically mypy, but also black. It's very annoying to learn about mistakes only when you are ready to commit (especially when types are involved). If this is something we want to do, a documentation about how to set it up in the VS Code or PyCharm should be part of it.

Before this all can happen, we should make sure all dependencies are up-to-date.

pabera avatar Jan 04 '23 22:01 pabera