[GitHub] [iceberg] samredai commented on a change in pull request #4285: Python: Add doctest to tox

[GitBox <gi...@apache.org>]

samredai commented on a change in pull request #4285:
URL: https://github.com/apache/iceberg/pull/4285#discussion_r822235103



##########
File path: python/setup.py
##########
@@ -19,7 +19,14 @@
 
 setup(
     name="py-iceberg",
-    install_requires=[],
+    install_requires=[

Review comment:
       These should go in setup.cfg under `[options]`. Did you get these from the dependencies from the legacy python client's setup.py? I think we should incrementally add dependencies as PR's require them. We also need to determine which are optional dependencies and how to group them. For now I think pyarrow is the only missing dependency:
   
   *setup.cfg*
   ```
   [options]
   ...
   install_requires =
       pyarrow
   ```
   As later PRs introduce optional dependencies, we can add them like so:
   ```
   [options.extras_require]
   boto =
       boto
   hive =
       hmsclient==0.1.1
   ```

##########
File path: python/src/iceberg/expressions.py
##########
@@ -53,7 +54,7 @@ class Operation(Enum):
     AND = auto()
     OR = auto()
 
-    def negate(self):
+    def negate(self) -> Operation:

Review comment:
       Can you change `-> Operation` to `-> "Operation"`? That would allow you to remove the `from __future__ import annotations` at the top.

##########
File path: python/src/iceberg/io/pyarrow.py
##########
@@ -43,9 +43,14 @@ class PyArrowFile(InputFile, OutputFile):
     Examples:
         >>> from iceberg.io.pyarrow import PyArrowFile
         >>> input_file = PyArrowFile("s3://foo/bar.txt")
-        >>> file_content = input_file.open().read()  # Read the contents of the PyArrowFile instance
+        >>> # Read the contents of the PyArrowFile instance
+        >>> # Make sure that you have permissions to read/write
+        >>> # file_content = input_file.open().read()

Review comment:
       This makes sense for now to get the checkdocs test passing but we talked about either mocking out S3 with some conftest monkeypatches or using a minio instance for more robustness. I'm not exactly sure if there's a way to inject mocks into checkdocs.




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