Add pre-commit tool

[KAUTH]

In this commit, we add the pre-commit tool to improve the Software Development Lifecycle of the project by automatically running checks (linting & formatting) before one commits their changes.

Specifically:

• black, flake8 and pylint were adjusted in tox to run as pre-commit hooks. Additionally, the check-added-large-files commit hook was added (prevents giant files from being committed).

• We fixed doc build and pylint failing:

• Docs were failing with the pinned versions of their dependencies (sphinx, sphinx-autodoc-typehints). So we unpinned the dependencies (also good as best practice).

• Pylint was not able to read the sections related to its configuration in tox.ini. So we added a pylint section in tox.ini with the necessary key/value entries.

• the snowflake-connector-python was needed as a dependency for pylint (otherwise we got an ImportError)

• We updated the CONTRIBUTING.md with how to install pre-commit

[KAUTH]

Hey @matthieucan thanks for taking a look at the PR!

If I understand correctly, this PR would make pre-commit the central tool for linting, where the linting configuration is held (as opposed to tox.ini currently).

Linting by CI or even by the developer would still be called by tox, e.g., with tox -e lint, which in its turn will trigger pre-commit to run. The same pre-commit commands will be triggered automatically before each commit (if the developer downloads and installs pre-commit). The configuration mainly remains in tox.ini. Only the call of the linters with their potential arguments, the files to scan and their dependencies are in the pre-commit configuration file (.pre-commit-config.yaml). Do you see any constraints with this implementation?

One advantage that I see in pre-commit ran as a linter (that I haven't applied by default to this PR yet) and that tox does not provide, is that you can run pre-commit only on the last commit (here I use for the first time pre-commit run --all-files). This is extremely fast and also gives you the chance to slowly migrate changes if you are adapting to another linting format or adding something totally new at some point in the future, without having to refactor the whole code at once for the linter checks to pass.

Do you think that'd be possible, e.g. by calling tox -e some-function from the pre-commit configuration file, or some other way?

I believe you could do something like that with pre-commit's "Repository local hooks".

[matthieucan]

Hi @KAUTH, apologies for the delay.

Linting by CI or even by the developer would still be called by tox, e.g., with tox -e lint, which in its turn will trigger pre-commit to run. The same pre-commit commands will be triggered automatically before each commit (if the developer downloads and installs pre-commit). The configuration mainly remains in tox.ini. Only the call of the linters with their potential arguments, the files to scan and their dependencies are in the pre-commit configuration file (.pre-commit-config.yaml). Do you see any constraints with this implementation?

One thing that I see would be great is for pre-commit to stay optional, i.e.

• linting configuration is stored in tox, and independent of pre-commit
• pre-commit can be used by developers, and it will trigger tox commands.

Does that make sense?

One advantage that I see in pre-commit ran as a linter (that I haven't applied by default to this PR yet) and that tox does not provide, is that you can run pre-commit only on the last commit (here I use for the first time pre-commit run --all-files). This is extremely fast and also gives you the chance to slowly migrate changes if you are adapting to another linting format or adding something totally new at some point in the future, without having to refactor the whole code at once for the linter checks to pass.

This is very interesting! But I don't really understand how it works. For example, when running pylint, what does it mean to only run it on the last commit? pylint needs the "full files", not just the last diff, in order to fully parse python modules. Or did I misunderstand this feature?

[KAUTH]

Hi @matthieucan, thanks for the reply. 😊

One thing that I see would be great is for pre-commit to stay optional, i.e. linting configuration is stored in tox, and independent of pre-commit

Could you give an example as to which linting configuration you are referring to? Because even with pre-commit the linters use tox.ini's configuration. The only things in pre-commits config file are optional arguments (if any) to the linters, the files scanned and the tools' dependencies. Configuration such as max-args, max-attributes of pylint or e.g., line_length for flake8, are still in the tox.ini.

pre-commit can be used by developers

yes, that is true however in both use-cases (tox calling pre-commit and pre-commit calling tox)

This is very interesting! But I don't really understand how it works. For example, when running pylint, what does it mean to only run it on the last commit? pylint needs the "full files", not just the last diff, in order to fully parse python modules. Or did I misunderstand this feature?

Yes, you are right, pylint for example, would indeed need the whole file. I should have been more precise. pre-commit, if you choose it to, can run (it's the default option) on the changed files only (not the actual diff).

So if for instance, you are introducing a new tool, such as black or pylint, and you want to have a gradual migration and not have edits to almost all your project files to conform to the new style, you only need to apply the changes to the files that have been changed on your last commit.

Some other examples, if you want to check it out, of tox calling pre-commit can be seen in:

• the pylint repo, https://github.com/PyCQA/pylint/blob/main/tox.ini#L13
• the flask repo, https://github.com/pallets/flask/blob/main/tox.ini#L26
• the bandit repo, https://github.com/PyCQA/bandit/blob/main/tox.ini#L79

If however you still have some concerns, I can try to send a PR with pre-commit calling tox.

[matthieucan]

Hi @KAUTH,

One thing that I see would be great is for pre-commit to stay optional, i.e. linting configuration is stored in tox, and independent of pre-commit

Could you give an example as to which linting configuration you are referring to? Because even with pre-commit the linters use tox.ini's configuration. The only things in pre-commits config file are optional arguments (if any) to the linters, the files scanned and the tools' dependencies. Configuration such as max-args, max-attributes of pylint or e.g., line_length for flake8, are still in the tox.ini.

You're right about the configuration stored in tox.ini, I didn't mean to imply it's not the case in your PR. What I wanted to highlight is the dependency tox -> pre-commit, as seen in tox's configuration file.

So if for instance, you are introducing a new tool, such as black or pylint, and you want to have a gradual migration and not have edits to almost all your project files to conform to the new style, you only need to apply the changes to the files that have been changed on your last commit.

I see, that's pretty nice indeed. Thanks for the explanation. Looking at python's linter landscape however, I'm under the impression that most tools (except formatters like black) need to be able to import a full project in order to lint it. Moreover, having linters run only on new files is convenient for introducing them, but this does bring some inconsistency as some files would not adhere to new style rules.

If however you still have some concerns, I can try to send a PR with pre-commit calling tox.

Do you think that would be a lot of work?

[KAUTH]

Hey, I will try it out and send a new PR if it works fine 😊 Update: @matthieucan check out https://github.com/PicnicSupermarket/diepvries/pull/34.

[matthieucan]

Hey, I will try it out and send a new PR if it works fine blush Update: @matthieucan check out #34.

Looks very good, should we then close this PR?