jrouly commented on code in PR #490:
URL: https://github.com/apache/iceberg-python/pull/490#discussion_r1509828986


##########
pyiceberg/catalog/glue.py:
##########
@@ -252,6 +259,22 @@ def _construct_database_input(database_name: str, 
properties: Properties) -> Dat
     return database_input
 
 
+def _register_glue_catalog_id_with_glue_client(glue: GlueClient, 
glue_catalog_id: str) -> None:
+    """
+    Register the Glue Catalog ID (AWS Account ID) as a parameter on all Glue 
client methods.
+
+    It's more ergonomic to do this than to pass the CatalogId as a parameter 
to every client call since it's an optional
+    parameter and boto3 does not support 'None' values for missing parameters.

Review Comment:
   I could (very easily) be convinced that this is indeed _not_ more ergonomic, 
and that it makes more sense to add the actual `CatalogId` parameter to every 
single invocation of a Glue client call.
   
   That looks something like this:
   
   ```python
   def _glue_kwargs(self) -> dict[str, str]:
       if self.glue_catalog_id is not None:
           return dict(CatalogId=self.glue_catalog_id)
       return dict()
   
   ...
   
   # Add this kwarg expansion to every single client call.
   self.glue.get_databases(**self._glue_kwargs())
   ```
   
   It works, it just gets a little noisy.



-- 
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-unsubscr...@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org

Reply via email to