[GitHub] [iceberg] Fokko commented on a diff in pull request #7519: Python: Add REST support for SigV4 signed requests

["Fokko (via GitHub)" <gi...@apache.org>]

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


##########
python/pyiceberg/catalog/rest.py:
##########
@@ -172,8 +175,6 @@ class OAuthErrorResponse(IcebergBaseModel):
 
 class RestCatalog(Catalog):
     uri: str
-    session: Session

Review Comment:
   In Python it depends on the context:
   
   By referencing it using `self`, it is bound to the object itself:
   
   ```python
   class SomeClass:
       foo: str
       def __init__(self, foo: str):
           self.foo = foo
   
   >>> class SomeClass:
   ...     foo: str
   ...     def __init__(self, foo: str):
   ...         self.foo = foo
   ... 
   >>> a = SomeClass('a')
   >>> a.foo
   'a'
   >>> b = SomeClass('b')
   >>> b.foo
   'b'
   >>> a.foo
   'a'  # still holds a
   ```
   
   Class-level attributes are referenced by the class itself.
   ```python
   >>> SomeClass.a = 'a'
   >>> SomeClass.b = 'a'
   >>> SomeClass.a
   'a'
   >>> SomeClass.b
   'b'
   >>> b.a
   'a'
   >>> SomeClass.c = 'c'
   >>> a.c
   'c'
   ```
   
   I would suggest setting this to:
   
   ```python
       _session: Session
   ```
   
   It is not strictly required, but it helps the developers to indicate which variables are available. The underscore `_` indicates that it is private to the class.



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