Add black and isort as well as tox -e format command #3383
Conversation
| from iceberg.utils.bin_packing import PackingIterator | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("splits, lookback, split_size, open_cost", [ |
There was a problem hiding this comment.
I am usually hesitated to add any auto-formatting functionality to a project, because sometimes the formatting is quite unnecessary, such as here. But I don't know what's the convention on python side, maybe this is desirable. Any thoughts? @jun-he
There was a problem hiding this comment.
The consistency is nice because individual styles in python can vary wildly. The auto sorting of imports is also super helpful since it guarantees imports to be neatly organized as stdlib, third party, and package imports, each section separated by a single space. Since we recommend auto formatters on the java side and have style checks, has that been a net positive?
There was a problem hiding this comment.
Cool, I am good with that then as long as it follows the general guideline of the language!
There was a problem hiding this comment.
Yeah black is definitely the standard in the python community these days. I'm also generally usually opposed but in this case, I think it makes sense.
Admittedly, the choices it makes are definitely kind of strange to me at times. But I do recognize that it's the standard formatter for the language and is generally used as a best practice.
There was a problem hiding this comment.
👌 I feel it is good to have it here to help us to keep the code consistent. We can tune the settings to ensure it does the right thing.
|
Thanks for adding this @samredai. I've been doing black formatting locally. :) |
|
If @jun-he has no objections to the PR, we should merge it to help the python PRs and maintain a consistent formatting. LGTM otherwise |
kbendick
left a comment
kbendick
left a comment
There was a problem hiding this comment.
This looks good to me. Thanks for doing this.
| from iceberg.utils.bin_packing import PackingIterator | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("splits, lookback, split_size, open_cost", [ |
There was a problem hiding this comment.
Yeah black is definitely the standard in the python community these days. I'm also generally usually opposed but in this case, I think it makes sense.
Admittedly, the choices it makes are definitely kind of strange to me at times. But I do recognize that it's the standard formatter for the language and is generally used as a best practice.
python/tox.ini
Outdated
|
|
||
| [flake8] | ||
| ignore = E501,W503 | ||
| ignore = E501,E721,I100,I101,I202,W503 |
There was a problem hiding this comment.
nit: current build passed without ignoring E721,I100,I101,I202. We may consider to add those if needed in the future and keep it as is in this PR.
There was a problem hiding this comment.
The I ones are the suggested ones to prevent conflicting with isort and should also include I201 but I totally agree, let's add them when once they cause the tests to fail. Updating now.
EDIT: @jun-he I just ran tox and it did fail with I100 and I202 so I kept those in and removed the other two. Did you include the isort linting when you tested it out?
|
I'll wait for PR #3423 to get merged and I'll rebase this then rerun |
|
Thanks, @samredai! |
6 participants
This adds black and isort as well as an auto format command
tox -e formatthat formats the entire project. The only direct changes I made here is to thetox.inifile and the rest of the diff is from running the auto-formatter.