jazzband / tablib

Python Module for Tabular Datasets in XLS, CSV, JSON, YAML, &c.
https://tablib.readthedocs.io/
MIT License
4.61k stars 593 forks source link

Consider enforcing flake8, black and isort #401

Open jezdez opened 5 years ago

jezdez commented 5 years ago

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.

claudep commented 5 years ago

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.

hugovk commented 5 years ago

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/).

guptarohit commented 5 years ago

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.

hugovk commented 5 years ago

Please see PR #411.

peymanslh commented 1 year ago

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

claudep commented 1 year ago

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

hugovk commented 1 year ago

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.

peymanslh commented 1 year ago

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?

claudep commented 1 year ago

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

hugovk commented 1 year ago

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.

claudep commented 1 year ago

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

hugovk commented 1 year ago

Deal!