tablib icon indicating copy to clipboard operation
tablib copied to clipboard

Consider enforcing flake8, black and isort

Open jezdez opened this issue 6 years ago • 12 comments

As part of #398 I've added disabled calls to flake8 including a commented-out installation of the flake8-black and flake8-isort plugins.

I would suggest to run black and isort once over the code base and enabling both plugins to have a positive long-term effect on code quality. I didn't make this part of #398 since it's not a hard requirement for Jazzband projects (yet) but I've seen more and more projects move to that strategy.

jezdez avatar Oct 18 '19 19:10 jezdez

I'm used to flake8 and isort by working on Django, but I'm not happy at all with black (even if it will come to Django too). At some point, with the accumulation of those code "helper" tools, I'm feeling like that's imposing some sort of "slavery" on developers. However, I understand this is not my project and will accept what the majority prefers.

claudep avatar Oct 18 '19 19:10 claudep

I like Black, as I feel like it's releasing developers from having to worry about any formatting discussions. Just run a tool and it's done automatically. (Same for isort.)

I've also started using pre-commit too, so you don't need to worry about it. A good thing about that, if you don't like pre-commit, it's completely optional and developers aren't required to use it. And it can be a really handy way to group linters together into a single command on the CI (for example, see linting in tox.ini at https://github.com/hugovk/pypistats/).

hugovk avatar Oct 18 '19 20:10 hugovk

I've started using black+flake8+isort with pre-commit hooks in django projects. Things are better now! :) I second the idea using this combination.

guptarohit avatar Oct 20 '19 17:10 guptarohit

Please see PR #411.

hugovk avatar Oct 22 '19 09:10 hugovk

I ran flake8 on the src directory and it raised some issues. Can I fix some of them and send a PR?

peymanslh avatar Feb 02 '23 14:02 peymanslh

I thought flake8 was run through pre-commit. @hugovk, isn't it the case?

claudep avatar Feb 02 '23 18:02 claudep

No, we fixed some Flake8 things but because we decided not to use Black there were still some formatting-related ones to fix.

So a PR to fix more would be welcome!

And let's add Flake8 to pre-commit.

We don't necessarily have to fix all the Flake8 right now (but it'd be nice!), we can temporarily exclude some rules and fix those later. It would help prevent new ones slipping in.

hugovk avatar Feb 02 '23 18:02 hugovk

Thanks! I'll add flake8 to pre-commit and fix some issues in 2 separate PRs. What value should I set for max_line_length?

peymanslh avatar Feb 02 '23 20:02 peymanslh

I like Django's limit (119), but some judge it too high.

claudep avatar Feb 02 '23 21:02 claudep

That is a bit too high for me :) I often diff two things side by side, that would cause a lot of wrapping.

Shall we use 88? It's also the Black default, to make things easier in case we decide to adopt it in the future?

Still bigger than the Flake8 default 79.

hugovk avatar Feb 02 '23 21:02 hugovk

99 and call it a day (I like selling carpets :nerd_face: )

claudep avatar Feb 02 '23 21:02 claudep

Deal!

hugovk avatar Feb 02 '23 22:02 hugovk