Review Python linting strategy and possible automation

[vsoch]

No description provided.

[jameshcorbett]

Thanks for making an issue. I didn't have any input on the Python formatting rules in this project (they started before I joined I think) and I also found them pretty annoying when I started, but now I don't really mind them.

Now my opinion is that pylint is and always will be annoying (although useful) but black is fine. And I don't mind hard line limits if they're generous enough. I also wouldn't mind changing out black for a more generic PEP8 checker (rather than an auto formatter)---I would still use black to satisfy it though.

I don't write a ton of code in this repo but if we were going to have project-wide standards then I would probably want to spend more time thinking about it. And probably @cmoussa1 would too?

[cmoussa1]

Thanks for tagging me in this @jameshcorbett. I think out of the two Python formatters, I prefer black. I agree with James that pylint can sometimes be a pain, but it is useful to have while writing Python code. I often catch myself making dumb mistakes that get revealed when I run pylint, so to me, I like having both. I don't have a lot of experience contributing to Python projects outside of Flux, so having these two formatters run at the same time on my code is just something I learned to adapt and accept, and never really gave it any thought (but maybe it comes with my own naivety). :P

It probably warrants some more discussion, and whatever we end up deciding, in my opinion, it would be best for the rest of the sub-projects to follow suit and maintain consistency with flux-core, as James mentioned above. It's probably worth mentioning that I simply copied flux-core's Python formatting guidelines when the flux-accounting repository was first created.

[jameshcorbett]

Tagging #4623 (comment) for reference

[garlick]

Note that we did actually put down "black" as our only project-wide python "standard" in RFC 7

We should change RFC 7 to whatever makes sense and doesn't scare people away! I think we chose black just because nobody had a strong feeling about python style and black doesn't offer options, so no decisions were needed :_)

I could be wrong. Maybe @cmoussa1 has been a ball of rage all this time.

[grondo]

IIRC, @tgamblin had suggested black

[tgamblin]

Black's nice b/c it's fully automatic -- i.e., nobody has to bother fixing formatting issues -- at least not black ones. On Spack, we use these (in this order):

1. isort: nice for the same reason -- it keeps imports ordered nicely and it fixes automatically.
2. mypy: has grown on me though it took some work to transition to (thanks @trws!)
3. black: again, fully automatic. Just deal with it. It's nice.
4. flake8: super-helpful even if you use black because it does things like finding unused variables in code. Most of the annoying flake8 errors are automatically fixed by black (except some of the really long lines that black can't fix b/c it won't do things like breaking strings).

We waited to switch to black until we had a nice bot so that maintainers would not need to install anything local to use it. See, for example, spack/spack#29761 (comment) -- you say spackbot fix style and the bot pushes to your PR as you. That smoothed the transition but the bot took a bit of work to get right. If you don't mind bots stealing commits, you might try pre-commit CI, as it may be simpler to integrate.

I use blacken.el in emacs so that when I save files they're automatically blackened, and it works great. I added support to it so that it reads your pyproject.toml and only blackens if black is enabled for your project. I think there are similar things for vim.

There is also the very popular pre-commit tool, which applies black and other style checks automatically locally: https://pre-commit.com

[tgamblin]

Oh and in case you think we went to black unscientifically 🤪, I did a whole analysis of what our line length should be in spack/spack#24718. We went with 99 -- see the plots there for the rationale. It MAY be a little OCD, just maybe.

[vsoch]

Nice discussion! I left for my run and this was blank and now 😍 ! Thanks for that, and I'm glad to see that I'm not out in left field with respect to my thoughts. I'm going to put this all together and will make some automation around the preferences soon - I'm also thinking isort, mypy, black, and flake8 (with a specific set of things) and a pre-commit that does nice cleanup of line endings, etc. (those always get me!) I think we can find a nice balance of automated fixes, but still having a good style enforced.

[vsoch]

And the only other one I use for other projects (that I don't see here) is pyflakes - and that might be fulfilled by a subset of flake8 (it finds unused variables/imports etc.) I'll test that out too in this context.

[tgamblin]

From here:

If you like Pyflakes but also want stylistic checks, you want flake8, which combines Pyflakes with style checks against PEP 8 and adds per-project configuration ability.

Maybe we should be using pylint instead of flake8 now that we use black for style. Not sure it's worth reworking CI over.

[vsoch]

I think I mostly have been using pyflakes because it sounds like some kind of breakfast cereal, and this is a highly important decision point! 🥄

[tgamblin]

pyflakes : corn flakes :: flake8 : product19

[vsoch]

product19... one step away from soylent green!

[vsoch]

Python project linting.... tools? No... it's people! 😱 🙀 😭

[trws]

Heh, I came to give a rundown and @tgamblin beat me to it. The thing that flake8 does that's really helpful, is that it's a meta-tool. It actually runs pylint as well as several others, and offers a common interface for enforcing or ignoring violations from any of them. I'm strongly in favor of running at least flake8, black, and mypy or pyright (for type checks, since we're on 3.6 we might as well get real type checking) all at a pinned versions easily fetched from a requirements file or similar so we don't run into a split between local dev and CI, and can easily keep things up to date.

The only real change in there would be we'll get slightly more tests, and probably want to come up with a list of the things we want to ignore. Spack's list of ignores is pretty good, though there are a couple we probably don't want to copy over: https://github.com/spack/spack/blob/develop/.flake8#L20

[vsoch]

Nice! So I had quite an adventure this morning - either flake8 fixes and/or isort completely broke testing, and a made a new PR that (finally) has the refactored pre-commit, only with black and mypy and general file cleanups (but flake8 and isort would need to be tested individually to be added back).

#4636

My gut is that it's import order that matters - so my proposal for next steps is to get that PR reviewed and merged, and then I can make incremental fixes for flake8 / isort to figure out what was the culprit that broke tests (and document it for future developers to not make the same mistake). Then we can further tweak our linting/formatting preferences from there!

But I am loving having added pre-commit, and just one place in the CI to look for linting and formatting checks. Separation I think only makes sense if the checks are time intensive, but in this case it was just #toomanyclicks!

[vsoch]

My strategy won't work - 3.6 is too old. Someone else feel free to tweak the current hard coded scripts to do differently, and ping me (or I'll remember and come back) when we can use a more up-to-date version of Python.