rdblue commented on code in PR #5488:
URL: https://github.com/apache/iceberg/pull/5488#discussion_r947173354


##########
python/pyiceberg/catalog/__init__.py:
##########
@@ -53,7 +54,15 @@ class Catalog(ABC):
 
     def __init__(self, name: str | None, **properties: str):
         self.name = name
-        self.properties = properties
+
+        if name is not None:

Review Comment:
   URI isn't specific to REST. JDBC and Hive also use URI. Credential is 
REST-specific though.
   
   For the environment variables, we can always remove them and add them back 
when we find a good way to manage them. We just need to deconflict the existing 
ones (`PYICEBERG_URI`, etc.) and the new-style ones 
`PYICEBERG__CATALOG__NAME__URI` right?
   
   I think we have two options:
   1. `PYICEBERG_URI` configures an anonymous catalog with a random name that 
can only be used if you don't specify `--catalog`
   2. Remove `PYICEBERG_URI` and only support `PYICEBERG__*` properties
   
   I don't really mind either way. If we're going to support the `PYICEBERG__*` 
properties in general then it probably makes sense to get rid of the existing 
ones.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to