zopefoundation / zope.exceptions

exceptions and implementations which are general purpose
https://zopeexceptions.readthedocs.io/
Other
0 stars 6 forks source link

Add tox -e flake8, fix flake8 errors #14

Closed mgedmin closed 4 years ago

mgedmin commented 4 years ago

I've found a better way to suppress unused import warnings in dunder-init dot py: by defining __all__ to document the API of the package.

(Also I kind of hate how zope.exceptions randomly re-export a bunch of exceptions from zope.security, but only if that module is available.)

jamadden commented 4 years ago

In https://github.com/zopefoundation/meta/issues/6 it was decided not to automatically use black to format code. It feels like adding a CI job that enforces certain style decisions without also adding tooling to implement those style decisions is almost a worst-of-both-worlds situation. People will inevitably write code that fails the CI job and then have to tweak it manually to pass (a pre-commit hook to run some unspecified formatter does no good because tox is often run before any commits happen). That sounds frustrating.

I do have CI jobs that run pylint in some of my projects like gevent. I'm reconsidering that a bit now. Off the top of my head, I can only think of a handful of times a PR has failed because of a pylint error; of course it never effects me because I have my editor do that and don't save files with errors so I haven't found it frustrating, but I should think about keeping barriers as low as possible. I have many of the minor whitespace related checks disabled, so the checks that are enabled are mostly about semantics; black or some other formatter wouldn't do anything about those.

jugmac00 commented 4 years ago

It feels like adding a CI job that enforces certain style decisions without also adding tooling to implement those style decisions is almost a worst-of-both-worlds situation.

Isn't that what we have for most Zope projects? Ie flake8 as a Travis job? I have seen countless "fix flake8" commits spread over all projects.

(a pre-commit hook to run some unspecified formatter does no good because tox is often run before any commits happen)

You could create a tox "job" for the linter/formatter? Or even let tox run pre-commit?

There are soo many combinations... my head is spinning :-)

mgedmin commented 4 years ago

Isn't that what we have for most Zope projects? Ie flake8 as a Travis job?

Out of 353 repositories with a .travis.yml only 26 mention flake8, so "most" is a bit inaccurate.

(There are 0 that mention pylint, so we could say flake8 is the most commonly used linter in zopefoundation Travis jobs)

I have seen countless "fix flake8" commits spread over all projects.

I tend to fix flake8 warnings before working on a project because my editor highlights flake8 warnings, and those highlights annoy me (which is the entire point).

There are soo many combinations... my head is spinning :-)

Which is why we don't have automated enforcement of style/formatting in ZF repos, and that's okay.

icemac commented 4 years ago

With https://github.com/zopefoundation/meta/issues/13 we could put flake8 tests into more repositories but also try to add some tooling (e. g. as a tox environment) to automatically clean flake8 violations.