[GitHub] [iceberg] Fokko commented on a diff in pull request #5594: Python: Fix CLI after properties refactor

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

Fokko commented on code in PR #5594:
URL: https://github.com/apache/iceberg/pull/5594#discussion_r950804426


##########
python/pyiceberg/catalog/rest.py:
##########
@@ -422,7 +424,6 @@ def list_namespaces(self, namespace: Union[str, Identifier] = ()) -> List[Identi
             ),
             headers=self.headers,
         )
-        response.raise_for_status()
         namespaces = ListNamespaceResponse(**response.json())

Review Comment:
   We should move this one after the `raise_for_status` block, otherwise, we might consume the payload before checking if it is a 2xx. A non 2xx would have a different structure, and we would fail with a pedantic error.



##########
python/pyiceberg/cli/output.py:
##########
@@ -134,6 +140,9 @@ def uuid(self, uuid: Optional[UUID]):
 class JsonOutput(Output):
     """Writes json to stdout"""
 
+    def __init__(self, **properties: Any):

Review Comment:
   We could also move this to the constructor of the parent `Output` class, and then we don't have to implement it at both the `JsonOutput` and `ConsoleOutput`.



##########
python/pyiceberg/catalog/rest.py:
##########
@@ -179,10 +179,9 @@ def __init__(
             name: Name to identify the catalog
             properties: Properties that are passed along to the configuration
         """
-        super().__init__(name, **properties)
-
-        self.uri = self.property("uri")
-        if credential := self.properties.get("credential"):
+        self.properties = properties

Review Comment:
   Ah, thanks. Great catch! The problem here is that the `super().__init__` is only being called once, and it silently ignores the call on line 186 with the updated properties (that has the token).



##########
python/tests/catalog/test_rest.py:
##########
@@ -64,14 +64,9 @@ def rest_mock(requests_mock: Mocker):
 
 
 def test_no_uri_supplied():
-    with pytest.raises(ValueError) as exc_info:
+    with pytest.raises(KeyError):
         RestCatalog("production")
 
-    assert (

Review Comment:
   This is okay with me, this is also covered by `tests.cli.test_console:test_missing_uri` which is user-facing.



##########
python/pyiceberg/cli/output.py:
##########
@@ -68,12 +68,18 @@ def uuid(self, uuid: Optional[UUID]):
 class ConsoleOutput(Output):
     """Writes to the console"""
 
+    def __init__(self, **properties: Any):
+        self.verbose = properties.get("verbose", False)

Review Comment:
   When we use class variables, it is best to also add it to the class itself:
   ```python
   class ConsoleOutput(Output):
       """Writes to the console"""
   
       verbose: bool
   ```



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