Adding pre-commit hooks

[khoda81]

This PR is a playground for testing different pre-commit hook configurations. Code style is subjective and I recommend testing different configs until you find your preferred style. Also, I am trying to only commit .pre-commit-config.yaml file to this PR for now, because running pre-commit does a lot of modifications all over the place and just makes it difficult to merge later on.

To Install

pip install pre-commit

To Run

pre-commit run --all-files

For more info about pre-commit check pre-commit.com and for more hooks check pre-commit.com/hooks.html.

[coveralls]

Coverage: 100.0%. Remained the same when pulling 4e40c2cd456b22d92df9654cbd94e0746a45c4f2 on khoda81:precommit-hooks into e8605ad36e0e180f453588a13dae039d3e68ce53 on caleb531:develop.

[coveralls]

Coverage: 100.0%. Remained the same when pulling 4e40c2cd456b22d92df9654cbd94e0746a45c4f2 on khoda81:precommit-hooks into e8605ad36e0e180f453588a13dae039d3e68ce53 on caleb531:develop.

[caleb531]

@khoda81 A few points of feedback:

1. If pre-commit should be installed as a Python dependency, then it should be added to the requirements.txt file (you can do this via pip freeze > requirements.txt so that dependencies are still sorted properly)
2. I see a lot of hooks in here for checkers that are not part of this project's development pipeline currently (e.g. we should be running mypy on pre-commit, not pyright). Also, we shouldn't need pycodestyle since we already have black
3. Because isort is part of flake8, we shouldn't need it in the VS Code config, especially since the source.organizeImports VSC setting will already take care of that; I would instead recommend replacing ms-python.isort with kevinglasson.cornflakes-linter

@eliotwrobson What do you think on all this?

[eliotwrobson]

@caleb531 sorry I missed your comment on this from before. I think that if we have precommit hooks set up that match the CI for this project (mypy, black, etc.), then I think it should be fine. I actually think it's a little bit overkill to do this on this project, since we already have CI checks that make sure these things work, and the autoformatting stuff happens with saving in VSCode already. So running stuff like mypy locally seems like a bit of a waste (and can be annoying on slower computers).

However, I don't feel particularly strongly either way.

[caleb531]

@eliotwrobson Yeah, I feel like adding a pre-commit infrastructure really only benefits the people not using VS Code or those ignoring the non-style warnings in VS Code (i.e. those things that black won't fix on save).

And for those that do follow the aforementioned practices, all those pre-commit checks will be redundant, and they may potentially even slow down development (e.g. gotta wait for these pre-commit checks to run on every commit). Which may discourage us (or others) from creating smaller, more focused commits because we get fatigued by the time spent waiting for those checks.

Maybe performance wouldn't be an issue in actuality or at least for most people, but I think the redundancy factor is still there.

[eliotwrobson]

@caleb531 I think we're mostly in agreement, not a huge preference either way but feels like a redundant change to make given there are already automated tools doing essentially the same thing. In the interest of keeping things focused on the v8 release, and the fact this PR hasn't seen attention in a while, I'm inclined to close it and reopen later if people feel like this is something they really want. Does this seem like a good course of action to you?

[caleb531]

@eliotwrobson I agree, and that plan sounds good to me.

@khoda81 Just wanted to say thank you for contributing this in the first place! We may revisit it in the future, but for the time being, I'm sorry to say it won't be something we'll be pursuing.