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 2022/05/18 17:24:16 UTC

[GitHub] [iceberg] Fokko opened a new pull request, #4811: Python: Replace tox by pre-commit

Fokko opened a new pull request, #4811:
URL: https://github.com/apache/iceberg/pull/4811

   I would like to suggest moving away from tox into pre-commit.
   
   A couple of things in this PR:
   
   - Moving away from tox to pre-commit. [Pre-commit](https://pre-commit.com/) is a very popular tool for running linting tools for Python.
     - I don't think tox isn't very effective (anymore) since most people use Docker or their system Python.
     - Pre-commit is very easy to understand and manage
   - Pin the versions of all the linters (instead of installing the latest and greatest before). Updating using pre-commit is very easy: `pre-commit autoupdate`.
   - Removed the duplicate test_raise_on_checking_if_local_file_exists_no_permission test: https://github.com/apache/iceberg/search?q=test_raise_on_checking_if_local_file_exists_no_permission This was caught now we also run the linters on the test codebase.
   - Updated the docs on how to run linting and testing.


-- 
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 diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r876579696


##########
python/tests/io/test_pyarrow.py:
##########
@@ -180,20 +180,6 @@ def test_raise_on_creating_a_local_file_no_permission():
         assert "Cannot get file info, access denied:" in str(exc_info.value)
 
 
-def test_raise_on_checking_if_local_file_exists_no_permission():
-    """Test that a PyArrowFile raises when deleting a local file without permission"""
-
-    with tempfile.TemporaryDirectory() as tmpdirname:
-        os.chmod(tmpdirname, 0o600)
-        file_location = os.path.join(tmpdirname, "foo.txt")
-        file_io = PyArrowFileIO()
-
-        with pytest.raises(PermissionError) as exc_info:
-            file_io.delete(file_location)
-
-        assert "Cannot delete file" in str(exc_info.value)

Review Comment:
   Question: Why is this test being removed?



-- 
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] dhruv-pratap commented on pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
dhruv-pratap commented on PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#issuecomment-1135018865

   I tried to test out the instructions in contribution.md. `make test` works fine, but `make lint` is skipping everything. I have feeling it might be something to do with the working directory settings that are environment specific.
   ```logs
   (venv) ➜  python git:(fd-move-to-pre-commit) make lint
   pre-commit run 
   trim trailing whitespace.............................(no files to check)Skipped
   fix end of files.....................................(no files to check)Skipped
   check docstring is first.............................(no files to check)Skipped
   debug statements (python)............................(no files to check)Skipped
   check yaml...........................................(no files to check)Skipped
   check python ast.....................................(no files to check)Skipped
   black................................................(no files to check)Skipped
   isort................................................(no files to check)Skipped
   mypy.................................................(no files to check)Skipped
   pycln................................................(no files to check)Skipped
   
   ```
   


-- 
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 #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
rdblue merged PR #4811:
URL: https://github.com/apache/iceberg/pull/4811


-- 
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 diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r876552517


##########
python/src/iceberg/expressions/literals.py:
##########
@@ -44,13 +44,7 @@
     TimeType,
     UUIDType,
 )
-from iceberg.utils.datetime import (

Review Comment:
   On further thought, if this is easily configurable I'd be open to it.
   
   Looking at all of the import changes, I do seem to prefer the old style more. But possibly we'll adjust?



-- 
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] Fokko commented on pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
Fokko commented on PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#issuecomment-1135141442

   @dhruv-pratap Thanks for giving it a try! By default, pre-commit will only check the changed files (based on git). I've changed this to check all the files every time (to avoid confusion). Since the codebase is still relatively small, it almost runs instantly.


-- 
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] dhruv-pratap commented on a diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
dhruv-pratap commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r879929816


##########
python/.pre-commit-config.yaml:
##########
@@ -0,0 +1,47 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+---
+files: ^python/

Review Comment:
   My bad, I didn't realize that. I think we can leave it like it is in that case. 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] dhruv-pratap commented on a diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
dhruv-pratap commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r879765105


##########
python/.pre-commit-config.yaml:
##########
@@ -0,0 +1,47 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+---
+files: ^python/

Review Comment:
   Do we need this? For example I use `python/` as my working directory and when I run the `make lint` command it causes it to skip everything. I understand the CI environment may need these paths so its fine to have it in  `.github/workflows/python-ci.yml` but is there a way to avoid this in here?



##########
python/pyproject.toml:
##########
@@ -20,4 +20,30 @@ requires = [
     "setuptools>=42",
     "wheel"
 ]
-build-backend = "setuptools.build_meta"
\ No newline at end of file
+build-backend = "setuptools.build_meta"
+
+[tool.black]
+line-length = 130
+target-version = ['py38']
+
+[tool.isort]
+src_paths = ["python/src/", "python/tests/"]

Review Comment:
   Should this be relative the `pyproject.toml` file i.e. `["src/", "tests/"]` , like we have it configured for `tool.coverage.run` below. 



-- 
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] dramaticlly commented on a diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
dramaticlly commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r876429194


##########
python/pyproject.toml:
##########
@@ -20,4 +20,27 @@ requires = [
     "setuptools>=42",
     "wheel"
 ]
-build-backend = "setuptools.build_meta"
\ No newline at end of file
+build-backend = "setuptools.build_meta"
+
+[tool.black]
+line-length = 130
+target-version = ['py38']

Review Comment:
   ~~curious to know why is this py38 instead of py37 (minimal version we support)?~~
   
   nvm, looks like I missed https://github.com/apache/iceberg/pull/4784



-- 
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] dramaticlly commented on a diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
dramaticlly commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r886153904


##########
python/Makefile:
##########
@@ -0,0 +1,28 @@
+# Licensed to the Apache Software Foundation (ASF) under one

Review Comment:
   I think this Makefile make our life easier, but I guess some of them can even go into pyproject.toml if used with [`taskipy`](https://github.com/illBeRoy/taskipy) for arbitrary task execution? 
   
   ```toml
   [tool.taskipy.tasks]
   lint = """
       task lint_isort &&
       task lint_black
   """
   lint_isort = "isort src tests"
   lint_black = "black src tests"
   
   coverage = """
   coverage run --source=src/ -m pytest tests/ &&
   coverage report -m --fail-under=90 &&
   coverage html &&
   coverage xml
   """
   ```
   
   It can also be used with poetry directly like 
   `poetry run task coverage`
   or simply
   `task coverage` if one don't use poetry



-- 
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] Fokko commented on a diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r876884526


##########
python/src/iceberg/expressions/literals.py:
##########
@@ -44,13 +44,7 @@
     TimeType,
     UUIDType,
 )
-from iceberg.utils.datetime import (

Review Comment:
   Hey @dramaticlly @kbendick thanks for the input, and I agree that breaking it up into multiple lines is more readable.
   
   This changed because I also set isort to a max line length of 130 chars. Now I've set it to when there are more than 4 imports, it will break it up into a multi-line statement.
   
   This is configurable by isort, and I've just updated the code accodingly. Black is totally fine with it, so that should be okay 👍🏻 



-- 
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 diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r876544211


##########
python/src/iceberg/expressions/literals.py:
##########
@@ -44,13 +44,7 @@
     TimeType,
     UUIDType,
 )
-from iceberg.utils.datetime import (

Review Comment:
   I agree in this case, but I'm of the opinion that if this is how `black` / the formatting tool formats, we should choose to opt for the chosen default styles unless the change is really glaring.
   
   I imagine there's a limit we can maybe configure but 4 is arguably enough to use the single line style (this case kind of straddles both single-line and multi-line style imo).
   
   My understanding is that this is an area where convention (as dictated by the tool) is usually preferred over configuration.
   
   EDIT: I misread the original diff (new computer and new color scheme on github). I do definitely prefer the old multi-line style too here, but not at the expense of adding an inline comment to stop auto-formatting. And generally avoiding changing the defaults is a good choice whenever possible.



-- 
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 diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r876544211


##########
python/src/iceberg/expressions/literals.py:
##########
@@ -44,13 +44,7 @@
     TimeType,
     UUIDType,
 )
-from iceberg.utils.datetime import (

Review Comment:
   I somewhat agree in this case, but I'm of the opinion that if this is how `black` / the formatting tool formats, we should choose to opt for the chosen default styles unless the change is really glaring.
   
   I imagine there's a limit we can maybe configure but 4 is arguably enough to use this format. At 6 or so I would definitely prefer this style.
   
   My understanding is that this is an area where convention (as dictated by the tool) is usually preferred over configuration.



-- 
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] Fokko commented on a diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r876887844


##########
python/tests/io/test_pyarrow.py:
##########
@@ -180,20 +180,6 @@ def test_raise_on_creating_a_local_file_no_permission():
         assert "Cannot get file info, access denied:" in str(exc_info.value)
 
 
-def test_raise_on_checking_if_local_file_exists_no_permission():
-    """Test that a PyArrowFile raises when deleting a local file without permission"""
-
-    with tempfile.TemporaryDirectory() as tmpdirname:
-        os.chmod(tmpdirname, 0o600)
-        file_location = os.path.join(tmpdirname, "foo.txt")
-        file_io = PyArrowFileIO()
-
-        with pytest.raises(PermissionError) as exc_info:
-            file_io.delete(file_location)
-
-        assert "Cannot delete file" in str(exc_info.value)

Review Comment:
   Because mypy detected that this test was in there twice:
   ```
   ➜  python git:(fd-move-to-pre-commit) ✗ pre-commit run --all-files
   trim trailing whitespace.................................................Passed
   fix end of files.........................................................Passed
   check docstring is first.................................................Passed
   debug statements (python)................................................Passed
   check yaml...............................................................Passed
   check python ast.........................................................Passed
   black....................................................................Passed
   isort....................................................................Passed
   mypy.....................................................................Failed
   - hook id: mypy
   - exit code: 1
   
   python/tests/io/test_pyarrow.py:182: error: Name "test_raise_on_checking_if_local_file_exists_no_permission" already defined on line 154
   Found 1 error in 1 file (checked 35 source files)
   
   pycln....................................................................Passed
   ```
   
   I think this test was copied a couple of times, to create different permutations of the original test, but it looks like they are identical:
   
   https://github.com/apache/iceberg/blob/bb08b0492a1fde898560d668edd24cb49d2b3cc2/python/tests/io/test_pyarrow.py#L155-L166
   
   https://github.com/apache/iceberg/blob/bb08b0492a1fde898560d668edd24cb49d2b3cc2/python/tests/io/test_pyarrow.py#L183-L194
   
   Now posting the tests here, I see that there are slight differences :)))) I'll rename the tests accordingly.
   



-- 
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] Fokko commented on a diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r879859796


##########
python/.pre-commit-config.yaml:
##########
@@ -0,0 +1,47 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+---
+files: ^python/

Review Comment:
   Hey @dhruv-pratap. Yes, we need this, otherwise, it will touch most of the repo (including all the Java files) because of trailing whitespaces and end-of-line checks. We could remove this, but then it should be a separate PR.
   
   It is skipping everything on your end because it will only check the files you've touched. Pre-commit does this based on git (it checks like `git status`). This is helpful when the project becomes bigger, and the checks take more time. Otherwise, we can also disable this by always running `pre-commit run --all-files`. Let me know what you think!



-- 
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] Fokko commented on a diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r879861982


##########
python/pyproject.toml:
##########
@@ -20,4 +20,30 @@ requires = [
     "setuptools>=42",
     "wheel"
 ]
-build-backend = "setuptools.build_meta"
\ No newline at end of file
+build-backend = "setuptools.build_meta"
+
+[tool.black]
+line-length = 130
+target-version = ['py38']
+
+[tool.isort]
+src_paths = ["python/src/", "python/tests/"]

Review Comment:
   You're right, 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 commented on pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#issuecomment-1142681150

   Looks great! I checked the CI runs and everything looks good. It's also easy to read the results so that's great. Thanks, Fokko!


-- 
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] dramaticlly commented on a diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
dramaticlly commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r876429194


##########
python/pyproject.toml:
##########
@@ -20,4 +20,27 @@ requires = [
     "setuptools>=42",
     "wheel"
 ]
-build-backend = "setuptools.build_meta"
\ No newline at end of file
+build-backend = "setuptools.build_meta"
+
+[tool.black]
+line-length = 130
+target-version = ['py38']

Review Comment:
   curious to know why is this py38 instead of py37 (minimal version we support)?



##########
python/src/iceberg/expressions/literals.py:
##########
@@ -44,13 +44,7 @@
     TimeType,
     UUIDType,
 )
-from iceberg.utils.datetime import (

Review Comment:
   I feel like the format before looks more clear to me, what do you think?



-- 
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 diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
samredai commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r879656488


##########
python/CONTRIBUTING.md:
##########
@@ -0,0 +1,44 @@
+<!--
+  - Licensed to the Apache Software Foundation (ASF) under one
+  - or more contributor license agreements.  See the NOTICE file
+  - distributed with this work for additional information
+  - regarding copyright ownership.  The ASF licenses this file
+  - to you under the Apache License, Version 2.0 (the
+  - "License"); you may not use this file except in compliance
+  - with the License.  You may obtain a copy of the License at
+  -
+  -   http://www.apache.org/licenses/LICENSE-2.0
+  -
+  - Unless required by applicable law or agreed to in writing,
+  - software distributed under the License is distributed on an
+  - "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+  - KIND, either express or implied.  See the License for the
+  - specific language governing permissions and limitations
+  - under the License.
+  -->
+
+# Contributing to the Iceberg Python libary
+
+## Linting
+
+We rely on `pre-commit` to apply autoformatting and linting:
+
+```bash
+pip install -e ".[dev,arrow]"
+make lint
+```
+
+By default, it only runs on the files known by git.
+
+Pre-commit will automatically fix the violations such as import orders, formatting etc. Pylint errors you need to fix yourself.
+
+In contrast to the name, it doesn't run on the git pre-commit hook by default. If this is something that you like, you can set this up by running `pre-commit install`.
+
+## Testing
+
+For Python, we use pytest in combination with coverage to maintain 90% code coverage
+
+```bash
+pip install -e ".[dev,arrow]"

Review Comment:
   Does it make sense to just add this to the targets in the `Makefile`?



-- 
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 diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
samredai commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r879664097


##########
python/src/iceberg/io/pyarrow.py:
##########
@@ -26,10 +26,15 @@
 from typing import Union
 from urllib.parse import urlparse
 
+from iceberg.io.base import (
+    FileIO,
+    InputFile,
+    InputStream,
+    OutputFile,
+    OutputStream,
+)
 from pyarrow.fs import FileInfo, FileSystem, FileType
 
-from iceberg.io.base import FileIO, InputFile, InputStream, OutputFile, OutputStream

Review Comment:
   It looks like the isort settings haven't been changed, but the autoformatter seems to have grouped local application imports with third party imports. Is there a way we make this stick to the [PEP8 imports](https://peps.python.org/pep-0008/#imports) recommendation?
   
   > Imports should be grouped in the following order:
   > 
   > 1. Standard library imports.
   > 2. Related third party imports.
   > 3. Local application/library specific imports.
   > 
   > You should put a blank line between each group of imports.



-- 
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 diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r876544211


##########
python/src/iceberg/expressions/literals.py:
##########
@@ -44,13 +44,7 @@
     TimeType,
     UUIDType,
 )
-from iceberg.utils.datetime import (

Review Comment:
   I agree in this case, but I'm of the opinion that if this is how `black` / the formatting tool formats, we should choose to opt for the chosen default styles unless the change is really glaring.
   
   I imagine there's a limit we can maybe configure but 4 is arguably enough to use the single line style.
   
   My understanding is that this is an area where convention (as dictated by the tool) is usually preferred over configuration.
   
   EDIT: I misread the original diff (new computer and new color scheme on github). I definitely prefer the old multi-line style too here, but not at the expense of adding an inline comment to stop auto-formatting. And generally avoiding changing the defaults is a good choice whenever possible.



-- 
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] dramaticlly commented on a diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
dramaticlly commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r886153904


##########
python/Makefile:
##########
@@ -0,0 +1,28 @@
+# Licensed to the Apache Software Foundation (ASF) under one

Review Comment:
   I think this Makefile make our life easier, but I guess some of them can even go into pyproject.toml if used with [`taskipy`](https://github.com/illBeRoy/taskipy) for arbitrary task execution? 
   
   ```toml
   [tool.taskipy.tasks]
   lint = """
       task lint_isort &&
       task lint_black
   """
   lint_isort = "isort src tests"
   lint_black = "black src tests"
   
   coverage = """
   coverage run --source=src/ -m pytest tests/ &&
   coverage report -m --fail-under=90 &&
   coverage html &&
   coverage xml
   """
   ```
   
   It can also be used with poetry directly like 
   ` poetry lint`
   or 
   `poetry coverage`



-- 
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] dramaticlly commented on a diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
dramaticlly commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r886184108


##########
python/Makefile:
##########
@@ -0,0 +1,28 @@
+# Licensed to the Apache Software Foundation (ASF) under one

Review Comment:
   I actually tried a bit and proposing it in #4923, please let me know if it makes any sense (can be thought of a overkill)



-- 
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] Fokko commented on pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
Fokko commented on PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#issuecomment-1142456149

   @dhruv-pratap I had to update some of the tests (because the Table object didn't have an identifier property). After fixing this, we got circular imports in the tests and the catalog class because of the annotation import. I've moved those to the `__init__.py` because aliasing types will never introduce any circular import if we keep them separate.


-- 
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 diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
samredai commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r879668588


##########
python/tests/io/test_pyarrow.py:
##########
@@ -139,7 +138,7 @@ def test_raise_on_opening_a_local_file_not_found():
 
 
 def test_raise_on_opening_a_local_file_no_permission():
-    """Test that a PyArrowFile raises appropriately when opening a local file without permission"""
+    """Test that a PyArrowFile raises appropriately when creating a local file without permission"""

Review Comment:
   I think this should be kept to "opening" right? The test is creating a temp directory with locked down permissions, and then asserting that the right exception is raised when trying to open a file in that directory.



-- 
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] Fokko commented on a diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r879852715


##########
python/CONTRIBUTING.md:
##########
@@ -0,0 +1,44 @@
+<!--
+  - Licensed to the Apache Software Foundation (ASF) under one
+  - or more contributor license agreements.  See the NOTICE file
+  - distributed with this work for additional information
+  - regarding copyright ownership.  The ASF licenses this file
+  - to you under the Apache License, Version 2.0 (the
+  - "License"); you may not use this file except in compliance
+  - with the License.  You may obtain a copy of the License at
+  -
+  -   http://www.apache.org/licenses/LICENSE-2.0
+  -
+  - Unless required by applicable law or agreed to in writing,
+  - software distributed under the License is distributed on an
+  - "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+  - KIND, either express or implied.  See the License for the
+  - specific language governing permissions and limitations
+  - under the License.
+  -->
+
+# Contributing to the Iceberg Python libary
+
+## Linting
+
+We rely on `pre-commit` to apply autoformatting and linting:
+
+```bash
+pip install -e ".[dev,arrow]"
+make lint
+```
+
+By default, it only runs on the files known by git.
+
+Pre-commit will automatically fix the violations such as import orders, formatting etc. Pylint errors you need to fix yourself.
+
+In contrast to the name, it doesn't run on the git pre-commit hook by default. If this is something that you like, you can set this up by running `pre-commit install`.
+
+## Testing
+
+For Python, we use pytest in combination with coverage to maintain 90% code coverage
+
+```bash
+pip install -e ".[dev,arrow]"

Review Comment:
   Great suggestion 👍🏻 



-- 
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] Fokko commented on a diff in pull request #4811: Python: Replace tox by pre-commit

Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #4811:
URL: https://github.com/apache/iceberg/pull/4811#discussion_r879861699


##########
python/src/iceberg/io/pyarrow.py:
##########
@@ -26,10 +26,15 @@
 from typing import Union
 from urllib.parse import urlparse
 
+from iceberg.io.base import (
+    FileIO,
+    InputFile,
+    InputStream,
+    OutputFile,
+    OutputStream,
+)
 from pyarrow.fs import FileInfo, FileSystem, FileType
 
-from iceberg.io.base import FileIO, InputFile, InputStream, OutputFile, OutputStream

Review Comment:
   Yes, this was actually caused by a misconfiguration in paths. I've reverted it and the import groups are nicely separated again. 
   
   I'm also happy to explicitly add a check on this: https://github.com/PyCQA/flake8-import-order but that might also be too much :)



-- 
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