This seems fine to me.

This isn't related to the other changes. I think it also will break the docs since they need to import everything to generate API docs.

I personally dislike pylint since it's (in my limited experience) too aggressive about its warnings and then the code tends to get littered with exclusion statements (or code gets factored oddly because it doesn't meet pylint's criteria). I find that's a barrier to contribution, but that's just my opinion.

Anyway, I just had one note in the tox.ini, but otherwise the change looks reasonable.

I'm not sure what all these are and looking through the pylint docs took a while, it'd be good to document them all in a comment.

Is pylint is ignoring all unused imports? If so, I think it's much better to explicitly mark unused imports to be ignored - it means the developers are aware it's not used and want it imported anyway.

Fair point. I copy it from Beaker and I think most of them are to make SQLALCHEMY happy.

No, it won't break the docs as it will install all the dependencies inherited from [testenv].

https://paste.fedoraproject.org/paste/5WrYJrXBfPnIEDuZo9gBNF5M1UNdIGYhyRLivL9gydE=

Right. I have explicitly set --disable=W to disable the “Unused import warning” warning. I think that is a fair point as I should explicitly mark unused imports to be ignored.

FYI you can also choose to ignore errors globally for flake8 so if you want to just ignore its complaints about block comments, you can ignore E265 (block comment should start with '# ') and E266 (too many leading '#' for block comment) by adding the following to tox.ini's flake8 block

# E265: block comment should start with '# '
# E266: too many leading '#' for block comment
ignore = E265, E266

@dcallagh, any input on this one?

I don't have any preference over pylint or flake8, so still :+1: from me.

Since there was discussion.. I guess it would be good to get another voice to weigh in on this one.

Matt actually posted this based on my suggestion in another PR...

@jcline I agree pylint at warning level is way too finicky. I feel the same about flake8 as well though.

A nice balance we struck in Beaker was to use pylint but only with errors enabled (no warnings) which means it basically just finds actual glaring problems like importing things that don't exist or referencing misspelt names or calling functions with wrong number of args.

Personally I find code style enforcers like flake8 to be a waste of time because they inevitably complain about code that is perfectly readable. If the aim is to ensure code is readable, then that's something that humans can better enforce in code review -- if the code is formatted in a way that hinders readability, then we'll point that out in review. And if the humans can read it then there is no issue.

Having said that, waiverdb is such a small code base right now that we could just leave flake8 turned on and just globally disable some of the clearly wrong checks like E265.

Either way, we really need to get it into Jenkins and ensure it stays passing on every PR, so that we don't have these "fix up random style problems for flake8" commits littering our PRs in future.

A related question... do we still need tox.ini? What purpose does it serve?

We should have a flake8/pylint step in our Jenkinsfile, and I guess a sphinx-build step as well... So we will end up maintaining the same build steps in both places.

I am assuming here that we can invoke tox itself in our Jenkinsfile because then it means every Jenkins run will be downloading piles of random stuff from the internet into a virtualenv which is not a sane build process.

Oops... I meant to say: I'm assuming that we cannot invoke tox in Jenkins

Personally I find code style enforcers like flake8 to be a waste of time because they inevitably complain about code that is perfectly readable. If the aim is to ensure code is readable, then that's something that humans can better enforce in code review -- if the code is formatted in a way that hinders readability, then we'll point that out in review. And if the humans can read it then there is no issue.

While that's true, the problem is everyone has different opinions about what's readable. Python has, for the most part, accepted PEP8 and I like enforcing it so everyone knows what to expect. I think having consistently formatted code is important not just for the sanity of the maintainers, but also to make the lives of contributors easier. No one wants to play "guess the style of the week" when working on code. The very fact that we don't have to have a human look at it to say whether or not it's readable is great (in my opinion). I don't want to start reviewing someone's code only to discover it's not readable. Machines are good at that, so let them enforce it.

A related question... do we still need tox.ini? What purpose does it serve?

It makes it easy for contributors to run the unit tests against many different versions of Python. Before I put it in place there were already incompatibilities with Python 3.

I am assuming here that we cannot invoke tox itself in our Jenkinsfile because then it means every Jenkins run will be downloading piles of random stuff from the internet into a virtualenv which is not a sane build process.

You can invoke tox in Jenkins (assuming Jenkins has tox installed which it probably should). Anitya does this in Travis, and a similar approach will work for Jenkins. Yes, you're not testing against the OS packages, but that's a good thing when you're running the tests against the pip-installed Python package. You're going to be running exactly what developers can run locally so there's none of this "Oh it works locally, but not in CI" stuff. You'll also catch incompatibilities with dependencies sooner rather than later.

You can (and should) run tests against the RPM version using its RPM dependencies, but that's a different thing.

pylint diverges from PEP-8 quite a bit, while flake8 is much closer to what PEP-8 says. Due to this, pylint usually requires a crazy amount of configuration for it to do anything sane, where flake8 just works out of the box. I highly recommend sticking with flake8.

Fair enough. I'm happy with Flake8. @dcallagh, what do you think?

@Jcline I don't think developers should be using random versions of dependencies in virtualenvs either though. If our production deployment is using system packages, then our tests must run against the same system packages, which means developers should also be developing against the same system packages.

My preferred approach for getting advance notice of dependency changes is to just run tests against rawhide (in a non-voting/non-blocking configuration).

Anyway we're now getting quite off-topic for this PR... I guess in the short term we just keep tox.ini and Jenkinsfile in sync.

@bowlofeggs you might have missed the fact that the pylint.rc in this patch is turning off all warnings, which turns off pretty much all of pylint's complaining about code style. It just finds actual mistakes instead, as I mentioned above.

Anyway, sounds like flake8 it is.

@mjia do you want to put up a patch which adds flake8 to the Jenkinsfile instead? I assume it's passing currently, or do we need any more fixes?

I might put up a separate PR for adding pylint as well (ignoring its warnings) and there are some other interesting static analysis tools we could use too, like I saw fm-orchestrator is using Bandit which I had never heard of before.

But before we go crazy adding more and more build-time checks I want to get Jenkins reporting results back on pull requests first.

@mjia do you want to put up a patch which adds flake8 to the Jenkinsfile instead? I assume it's passing currently, or do we need any more fixes?

The first problem is we need to get python-flake8 built into eng-rhel-7.

@mjia do you want to put up a patch which adds flake8 to the Jenkinsfile instead? I assume it's passing currently, or do we need any more fixes?

Done, see #28

Drop off this PR as we want to stick with Flake8.