[Proposal] Introduce style enforcement #61
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
From my experience it is important (if not vital) to keep code style consistent across the whole codebase. Consistency improves making assumptions about how things should work but also improves readability (and as we know - readability counts).
Currently, the repository has a mandatory black step on travis but this is something that happens in the background during CI and it seems that the black changes are not committed to the main branch (please correct me if I'm wrong here). Locally running black gives me
142 files would be reformatted, 27 files would be left unchanged.
.With this PR I would like to introduce enforcing a code style dictated (mostly) by PEP (through flake8) and black with small modifications and hook to pre-commit. This combination offers a quite easy transition and allows developers not to think/remember about formatting their code according to the guidelines.
Small explanations:
flake8 is a great toolkit for checking your code base against coding style (PEP8), programming errors (like “library imported but unused” and “Undefined name”) and to check cyclomatic complexity.
black is a code formatter. In essence it modifies the *.py files to be consistent with the defined rules. In this setup it would do the formatting in the background when commit command is executed.
auto-commit is a library that executes some code when the developer executes
git commit
command. For this change it means that auto-commit will triggerblack
andflake8
commands. If any of those commands return something else than 0 ('all good') the person will not be able to commit their changes (It is still possible to commit without style checking by adding--no-verify
to the command. ).After this PR is merged, each person who wants to commit to this repository would have to have
precommit
installed (possibly also flake8?). As you might notice, I have not changed any *.py files yet, this is because the checker and autoformatter works lazily - only on the files that will actually be committed. This means that the person who touches the file will be responsible to format it to a correct form if autoformatter is not decisive enough. I am happy to demonstrate this on one of the files if needed.Example:
I have added one space in
tests\tasks\test_multiplex_link_prediction.py
file, added this file bygit add *.py
and executedgit commit -m"Test"
command.Since there were failures, the code was not committed. Also, you can see that black modified the source code (
files were modified by this hook
). Now, it should be enough to manually fix flake8 errors (black ones were fixed automatically) and the commit should be good to go.At the beginning, this might mean that there will be many false positives because as of now the code does not adhere to PEP8 very well, but because of laziness, we are guaranteed that the change is gradual. However, it is important to introduce consistency as early as possible - the more code there is to fix the worse the experience.
The flake8 and black configurations in this PR are purely my preference, I am happy to change those and/or have a philosophical discussion on whether this is a good configuration. In all projects that I am involved with I am using (almost) exactly this configuration.