You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/10/27 00:37:18 UTC
[GitHub] [iceberg] samredai opened a new pull request #3383: Add black and isort as well as tox -e format command
samredai opened a new pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383
This adds black and isort as well as an auto format command `tox -e format` that formats the entire project. The only direct changes I made here is to the `tox.ini` file and the rest of the diff is from running the auto-formatter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#issuecomment-955774377
Thanks, @samredai!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on a change in pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
samredai commented on a change in pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#discussion_r739478011
##########
File path: python/tox.ini
##########
@@ -93,7 +125,7 @@ commands =
python -m http.server {posargs}
[flake8]
-ignore = E501,W503
+ignore = E501,E721,I100,I101,I202,W503
Review 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. Did you include the isort linting when you tested it out?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nssalian commented on pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
nssalian commented on pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#issuecomment-953283338
Thanks for adding this @samredai. I've been doing black formatting locally. :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#issuecomment-955763206
@samredai, since this is ready to go and #3423 requires changes is it okay to merge this? What is the benefit of waiting for #3423?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a change in pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#discussion_r737050357
##########
File path: python/tests/utils/test_bin_packing.py
##########
@@ -17,34 +17,76 @@
import random
-from iceberg.utils.bin_packing import PackingIterator
import pytest
+from iceberg.utils.bin_packing import PackingIterator
+
-@pytest.mark.parametrize("splits, lookback, split_size, open_cost", [
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
samredai commented on pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#issuecomment-955772887
@rdblue I think merging this makes sense since #3423 needs to remove `3.6`. I think the rest of what's there is additive i.e. adding mypy. So we can merge this and rebase the other PR. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nssalian commented on pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
nssalian commented on pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#issuecomment-953412964
If @jun-he has no objections to the PR, we should merge it to help the python PRs and maintain a consistent formatting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on a change in pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
samredai commented on a change in pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#discussion_r739478011
##########
File path: python/tox.ini
##########
@@ -93,7 +125,7 @@ commands =
python -m http.server {posargs}
[flake8]
-ignore = E501,W503
+ignore = E501,E721,I100,I101,I202,W503
Review 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. Did you include the isort linting when you tested it out?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#discussion_r738857698
##########
File path: python/tests/utils/test_bin_packing.py
##########
@@ -17,34 +17,76 @@
import random
-from iceberg.utils.bin_packing import PackingIterator
import pytest
+from iceberg.utils.bin_packing import PackingIterator
+
-@pytest.mark.parametrize("splits, lookback, split_size, open_cost", [
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on a change in pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
samredai commented on a change in pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#discussion_r739478011
##########
File path: python/tox.ini
##########
@@ -93,7 +125,7 @@ commands =
python -m http.server {posargs}
[flake8]
-ignore = E501,W503
+ignore = E501,E721,I100,I101,I202,W503
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jun-he commented on a change in pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#discussion_r739506240
##########
File path: python/tox.ini
##########
@@ -93,7 +125,7 @@ commands =
python -m http.server {posargs}
[flake8]
-ignore = E501,W503
+ignore = E501,E721,I100,I101,I202,W503
Review comment:
SG, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on a change in pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
samredai commented on a change in pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#discussion_r739478011
##########
File path: python/tox.ini
##########
@@ -93,7 +125,7 @@ commands =
python -m http.server {posargs}
[flake8]
-ignore = E501,W503
+ignore = E501,E721,I100,I101,I202,W503
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nssalian edited a comment on pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
nssalian edited a comment on pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#issuecomment-953412964
If @jun-he has no objections to the PR, we should merge it to help the python PRs and maintain a consistent formatting. LGTM from me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
samredai commented on pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#issuecomment-955088577
I'll wait for PR #3423 to get merged and I'll rebase this then rerun `tox -e format`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jun-he commented on a change in pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#discussion_r738970691
##########
File path: python/tests/utils/test_bin_packing.py
##########
@@ -17,34 +17,76 @@
import random
-from iceberg.utils.bin_packing import PackingIterator
import pytest
+from iceberg.utils.bin_packing import PackingIterator
+
-@pytest.mark.parametrize("splits, lookback, split_size, open_cost", [
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a change in pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#discussion_r737813322
##########
File path: python/tests/utils/test_bin_packing.py
##########
@@ -17,34 +17,76 @@
import random
-from iceberg.utils.bin_packing import PackingIterator
import pytest
+from iceberg.utils.bin_packing import PackingIterator
+
-@pytest.mark.parametrize("splits, lookback, split_size, open_cost", [
Review comment:
Cool, I am good with that then as long as it follows the general guideline of the language!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nssalian edited a comment on pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
nssalian edited a comment on pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#issuecomment-953412964
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jun-he commented on a change in pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#discussion_r738973596
##########
File path: python/tox.ini
##########
@@ -93,7 +125,7 @@ commands =
python -m http.server {posargs}
[flake8]
-ignore = E501,W503
+ignore = E501,E721,I100,I101,I202,W503
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on a change in pull request #3383: Add black and isort as well as tox -e format command
Posted by GitBox <gi...@apache.org>.
samredai commented on a change in pull request #3383:
URL: https://github.com/apache/iceberg/pull/3383#discussion_r737628796
##########
File path: python/tests/utils/test_bin_packing.py
##########
@@ -17,34 +17,76 @@
import random
-from iceberg.utils.bin_packing import PackingIterator
import pytest
+from iceberg.utils.bin_packing import PackingIterator
+
-@pytest.mark.parametrize("splits, lookback, split_size, open_cost", [
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org