Introduce Pre-commit hooks and Flake8

[matejcik]

i don't see any reason to introduce a pre-commit hook config that none of our tools use

also your changes are breaking black formatting.

i would accept a github action that checks the results of flake8, black, and possibly the other things you have set up

[willianpaixao]

Hi @matejcik, thanks for the quick feedback.

also your changes are breaking black formatting.

I saw no mention, in code or documentation in this repo that you are using Black for formatting. So I assumed your IDE was doing it, which most of them use Flake8.

i don't see any reason to introduce a pre-commit hook config that none of our tools use

That is a different discussion. Pre-commit hooks (either flake/black/even pytest) would increase reliability in the commits and PRs, especially here where there's no CICD testing the commits (#42 aims to fix that). So this PR is my kind suggestion to adopt this tool in this project. If Flake is the problem, I can refactor and replace for Black.

But closing the PR because the tool is not being used is like refusing a new feature because the very same feature is not yet implemented doesn't make sense.

Tell me your thoughts.

[matejcik]

I saw no mention, in code or documentation in this repo that you are using Black for formatting. So I assumed your IDE was doing it, which most of them use Flake8.

this is a good point, we should add a "code style" section to the main README

Pre-commit hooks (either flake/black/even pytest) would increase reliability in the commits and PRs

So there's two separate issues. One, pre-commit hooks introduce friction to the contributing process. This might be appropriate in many cases; however, you're basically asking the other contributors to change their workflow, and you're doing that by a PR with zero rationale for the change.

For the big Trezor repositories, the convention is that checks run in CI/CD, and setting up pre-commit hooks is voluntary. This repository is not one of the "big ones" so we don't have CI/CD in place -- thank you for the PR that enables the basics!

kind suggestion to adopt this tool in this project.

Second, I have no idea what "tool" you mean. I don't know what consumes the config that you're submitting - it's not git itself, I think? Is it maybe the github tool? This explanation is also completely missing from the PR. You're asking me and possible other contributors to use a particular tool, with no pros/cons and without even saying what tool :)

While I can appreciate providing nice defaults for something a lot of people are going to use (.editorconfig maybe?), I also don't want to stuff this repository full of configs for random tools a couple people somewhere are maybe using.

[willianpaixao]

however, you're basically asking the other contributors to change their workflow

As you said, the presence of the pre-commit config file has no effect until the developer runs pre-commit install in the repository root folder. So it wouldn't be forcing anyone.

and you're doing that by a PR with zero rationale for the change.

You're right, I should have explained the context.

Second, I have no idea what "tool" you mean. I don't know what consumes the config that you're submitting

First would be the pre-commit hook itself, which triggers automatic checks that happen upon every commit. This is a tool, I think.

Then we have the checks themselves, like checking for git conflicts, files with bad EOF (happens a lot in Open Souce repos and it's annoying to spot in some cases). But yes, I really should have explained them.

[matejcik]

I googled in the meantime and you seem to be referring to pre-commit. You should know that "pre-commit hook" is also a generic Git term, hence my confusion.