rpm-software-management / spec-cleaner

spec-cleaner
BSD 3-Clause "New" or "Revised" License
28 stars 34 forks source link

Enforce coding policy #274

Closed kstreitova closed 4 years ago

kstreitova commented 4 years ago

Use pre-commit tool for managing pre-commit git hook that will run black, flake8 and mypy checks before every commit.

pre-commit Every contributor has to install the pre-commit tool and then run pre-commit install for installing pre-commit hook to his/her git hooks.

Then pre-commit will run automatically on git commit and it will check the changed files with black, flake8, flake8-docstrings and mypy.

black The whole project was formated with black. pre-commit now will format all the new code in the pre-commit hook. If the code is needed to be formated, black formats the code and the check fails. So you have a chance to see the changes and commit again.

flake8-docstrings Currently, there is a lot of methods without docstrings. Since it checks all files involved in the commit, it may throw docstrings errors for the code that wasn't touched. Then it would be nice of you to help and add missing docstrings to make pre-commit happy. This is just a temporary problem because at some point I believe all methods will have their docstrings :)

Coding Guidelines In this PR I suggest enforcing Coding Guidelines (see README.md) during the code reviews and not accepting changes that don't meet it (mainly for docstrings for "non-public" methods that are not checked with flake8-docstrings extension).

Conan-Kudo commented 4 years ago

I'm generally not a fan of commit hooks. Why not just enforce them as tests in CI and give people a script they can run to easily check their code?

kstreitova commented 4 years ago

I'm generally not a fan of commit hooks. Why not just enforce them as tests in CI and give people a script they can run to easily check their code?

Yes, it should be in CI as well, that's the next step. In general, pre-commit hooks make things easier because all potential troubles are solved before PR so reviewers don't have to spend their time on commenting tests failed because of some missing space, wrong indentation or missing docstring and they can focus on code logic.

Moreover, tox (that used to run flake8 and mypy besides pytest) has been removed so pre-commit hook is a nice way how to have all these checks under one roof. It's also very easy to skip one or more checks if they don't suit you at the moment (e.g. WIP commit). The whole management (creating environment, updating, installing, configuration, ...) is much easier than shipping some kind of script, IMHO.

scarabeusiv commented 4 years ago

What the heck is with the python-coveralls and github actions, now it is borked for two weeks...

kstreitova commented 4 years ago

Ready to be merged.

Conan-Kudo commented 4 years ago

I generally have my misgivings about pre-commit and commit hooks, but this LGTM, so merging.