We are trying new linter on GitHub, because of several limitations of Hound - mainly because Hound ships with old version flake8 that doesn’t support newer python language constructs. We are testing WPS That looks really promising, but by default it is much stricter than Hound was.
PR implementing it is here, to test it you can just merge that branch inside yours and once you update your PR to develop, it will run.
So far it seems that it is much slower, but before we merge it, I would like to hear more opinions from developers.
WPS looks very nice!
I think we should also rely on black default linting with a config and the requirement the contributors run it on their PRs. This would imply a big linting commit on develop I think. black is a very good way to ensure code unification without requesting tedious changes from devs.
Yep, there is an option to hook in Black. What I am afraid is that it can be really restrictive (I mean the whole WPS stuff). One advatage of it is that is has its own pre-commit hooks so it won’t allow you to commit something that didn’t pass the checks, but until all rules ale fine-tuned, it can really slow things down.
IMO linting should never block committing. But it could raise warnings when a PR is set during the period of setting up the system. And when everything has been fine-tuned, it should block the PR which don’t match the requirements.
That is the trouble with pre-commit hooks. They are either on or off. The logic behind it is that the error the linter will show you in pre-commit hook will be eventually shown on github too, so there is no reason to force it. You should always be able to silence the linter with noqa if needed. But the truth is that the rules now are rather restrictive.
pre-commit hooks may process obvious linting (like trailing spaces or some indentation issues) but shouldn’t block committing.
the linting checks should be processed by github when a PR is set. During the beta stage, it’ll raise only warnings, until the appropriate settings are fine-tuned. Once it is considered stable, linting issues should prevent from merging.
noqa must be avoided as much as possible, they are the symptom of a badly configured linter and will spread like cockroaches faster than we can handle. In fact, if a pre-commit could block committing, I’d suggest it is if it finds any noqa…
Agreed - committing should be fine, merging PRs with it should not.
I agree to try and avoid noqa whenever we can. However, being able to noqa a specific check I think should be allowed in the rare cases that they are valid to ignore. So I’d say if noqa is used then # noqa: E731,E123 makes more sense to me than # noqa because it’s more explicit.
I’ve seen some linter checks like import maya being disallowed because the maya package does not exist, but in maya it does. So we’d then require to ignore those for example.
I do always have a hard time just finding the right check code for Hound. Not sure if WPS is better in that regard with actually mentioning the check code.
WPS is just plugin to flake8 and flake8 is used by Hound so it is the same thing. It is of course question of IDE support too: you can easily integrate it to whatever IDE you have and run it on file/folder/selection and it should print out errors with their code. But what I am missing is some centralized database about what every individual code means and why it is important. You can ofc google it and get good answers but there are various sources.
I guest giving support to run it in IDE is enough so it can be removed from pre-commit. There is already rule on GH that it won’t allow to merge anything with unresolved issues.
Has there been any consideration on tweaking the line length to increase it slightly? E.g. Black’s default line length of 88 which @fabiaserra mentions here coudl be nice to adopt.
I we were talking about it some time ago, but didn’t come up with resolution - if I remember correctly, we agreed that we shouldn’t drop the line restriction or make it like 120 characters because we still want to use more tabs opened horizontally in IDE. But I guess 88 characters are ok.