Add pre-commit config

[alexrudd2]

Works for me, adds about 0.3s to commit times.

[janiversen]

It seems you activated some new checks, please make sure that CI have the same checks. .precommit should be a subset of CI, not the other way round.

[janiversen]

Seems CI do not like the pre commit version

[janiversen]

Let me just explain why it is important that pre-commit is a subset and not a superset.

I often make small changes to a PR directly in github, and some users make pull requests without having the full development environment installed.

If precommit makes checks that CI does not, these would not be done in the above cases, and we would then run the risk of seeing the effect on the next commit we make.

I do believe running black and isort at least in a precommit is a very good thing!

You have removed empty lines at the end in some files, I hope that is not a demand because I prefer to end the file with an empty line (and some of my editors actually do it automatically).

You need to update dev.

[alexrudd2]

Seems CI do not like the pre commit version

Fixed the typo in requirements.txt

You have removed empty lines at the end in some files, I hope that is not a demand because I prefer to end the file with an empty line (and some of my editors actually do it automatically).

This was the pre-commit default (end-of-file-fixer). I'm surprised your editor adds an entire empty line (i.e. two newlines after the last character; mine only adds single newline.). Nevertheless, I disabled that hook.

You need to update dev.

I use rebase + --force-with-lease, you merge dev into the branch. They have the same effect :). There was no merge conflict anyways.

[janiversen]

The biggest problem is that precommit catches things that our CI do not, that is the wrong way round. A precommit must be a subset (or equal) to CI, because it is not guaranteed that a pull request have passed the pre-commit.

[janiversen]

Please do not misunderstand, I am all for a precommit hook, I have had a hook installed that does pylint/black/flake8 and sort for a long time.

But if we add it to the repo, we need to ensure there are no unexpected effects.

[janiversen]

Seems CI do not like the pre commit version

Fixed the typo in requirements.txt

You have removed empty lines at the end in some files, I hope that is not a demand because I prefer to end the file with an empty line (and some of my editors actually do it automatically).

This was the pre-commit default (end-of-file-fixer). I'm surprised your editor adds an entire empty line (i.e. two newlines after the last character; mine only adds single newline.). Nevertheless, I disabled that hook.

You need to update dev.

I use rebase + --force-with-lease, you merge dev into the branch. They have the same effect :). There was no merge conflict anyways.

The difference is that when GitHub claim "This branch is out-of-date with the base branch", the PR cannot be merged.

We never merge, without the CI giving green light when the PR is run on the latest dev.

[janiversen]

Just a thought.

[alexrudd2]

The difference is that when GitHub claim "This branch is out-of-date with the base branch", the PR cannot be merged.

This is an adjustable Branch protection rule, and not a GitHub default.

I neither have access to this setting, nor care what its value is for this repo.

[jamesbraza]

Happy to see this PR!

[janiversen]

The difference is that when GitHub claim "This branch is out-of-date with the base branch", the PR cannot be merged.

This is an adjustable Branch protection rule, and not a GitHub default.

I neither have access to this setting, nor care what its value is for this repo.

You should care, not about the setting, but about the effect. This setting is enabled to ensure every PR is tested against the current dev, which is a simply way to avoid conflicts on dev. Locally I do not merge but run “git rebase dev”, but github only offers the merge option, which is why you sometimes see a merge commit.

[janiversen]

I am starting to think about releasing 3.2, it would be nice to have this merged before.

[alexrudd2]

The difference is that when GitHub claim "This branch is out-of-date with the base branch", the PR cannot be merged.

This is an adjustable Branch protection rule, and not a GitHub default.
I neither have access to this setting, nor care what its value is for this repo.

You should care, not about the setting, but about the effect. This setting is enabled to ensure every PR is tested against the current dev, which is a simply way to avoid conflicts on dev. Locally I do not merge but run “git rebase dev”, but github only offers the merge option, which is why you sometimes see a merge commit.

Yes, I understand the effect. As I said, I use git rebase dev && git push --force-with-lease which is the same as you merging via GitHub.

[janiversen]

LGTM. Thanks