You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "jayceslesar (via GitHub)" <gi...@apache.org> on 2023/09/13 23:19:48 UTC
[GitHub] [iceberg] jayceslesar opened a new pull request, #8558: Python: Add `__repr__` for `Table` and `Catalog`
jayceslesar opened a new pull request, #8558:
URL: https://github.com/apache/iceberg/pull/8558
Closes #8295
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #8558: Draft: Python: Add `__repr__` for `Catalog`
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8558:
URL: https://github.com/apache/iceberg/pull/8558#discussion_r1332158553
##########
python/pyiceberg/catalog/__init__.py:
##########
@@ -600,3 +600,7 @@ def _get_updated_props_and_update_summary(
)
return properties_update_summary, updated_properties
+
+ def __repr__(self) -> str:
+ """Returns the string representation of the Catalog class."""
+ return f"{self.__class__} with name: {self.name} and properties: {self.properties}"
Review Comment:
I think this is great, but I'm hesitant to print the properties as well because it might expose credentials that might end up in logs.
--
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
[GitHub] [iceberg] jayceslesar commented on pull request #8558: Draft: Python: Add `__repr__` for `Catalog`
Posted by "jayceslesar (via GitHub)" <gi...@apache.org>.
jayceslesar commented on PR #8558:
URL: https://github.com/apache/iceberg/pull/8558#issuecomment-1731633978
> Thanks for fixing this @jayceslesar Much appreciated!
@Fokko Absolutely! Thanks for the review and helping gear me towards contributing more in the future
--
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
[GitHub] [iceberg] Fokko commented on pull request #8558: Draft: Python: Add `__repr__` for `Catalog`
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #8558:
URL: https://github.com/apache/iceberg/pull/8558#issuecomment-1731625021
Thanks for fixing this @jayceslesar Much appreciated!
--
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
[GitHub] [iceberg] jayceslesar commented on a diff in pull request #8558: Draft: Python: Add `__repr__` for `Catalog`
Posted by "jayceslesar (via GitHub)" <gi...@apache.org>.
jayceslesar commented on code in PR #8558:
URL: https://github.com/apache/iceberg/pull/8558#discussion_r1334418249
##########
python/pyiceberg/catalog/__init__.py:
##########
@@ -600,3 +600,7 @@ def _get_updated_props_and_update_summary(
)
return properties_update_summary, updated_properties
+
+ def __repr__(self) -> str:
+ """Returns the string representation of the Catalog class."""
+ return f"{self.__class__} with name: {self.name}"
Review Comment:
Definitely cleaner!
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #8558: Draft: Python: Add `__repr__` for `Catalog`
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8558:
URL: https://github.com/apache/iceberg/pull/8558#discussion_r1334500044
##########
python/tests/catalog/test_base.py:
##########
@@ -598,3 +598,8 @@ def test_add_column_with_statement(catalog: InMemoryCatalog) -> None:
schema_id=0,
identifier_field_ids=[],
)
+
+
+def test_catalog_repr(catalog: InMemoryCatalog) -> None:
+ s = catalog.__repr__()
Review Comment:
The Pythonic way of doing this is:
```suggestion
s = repr(catalog)
```
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #8558: Draft: Python: Add `__repr__` for `Catalog`
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8558:
URL: https://github.com/apache/iceberg/pull/8558#discussion_r1334533320
##########
python/pyiceberg/catalog/__init__.py:
##########
@@ -600,3 +600,7 @@ def _get_updated_props_and_update_summary(
)
return properties_update_summary, updated_properties
+
+ def __repr__(self) -> str:
+ """Returns the string representation of the Catalog class."""
Review Comment:
```suggestion
"""Return the string representation of the Catalog 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
[GitHub] [iceberg] Fokko commented on a diff in pull request #8558: Draft: Python: Add `__repr__` for `Catalog`
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8558:
URL: https://github.com/apache/iceberg/pull/8558#discussion_r1334240023
##########
python/pyiceberg/catalog/__init__.py:
##########
@@ -600,3 +600,7 @@ def _get_updated_props_and_update_summary(
)
return properties_update_summary, updated_properties
+
+ def __repr__(self) -> str:
+ """Returns the string representation of the Catalog class."""
+ return f"{self.__class__} with name: {self.name}"
Review Comment:
What do you think of?
```suggestion
return f"{self.name} ({self.__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
[GitHub] [iceberg] jayceslesar commented on pull request #8558: Python: Add `__repr__` for `Table` and `Catalog`
Posted by "jayceslesar (via GitHub)" <gi...@apache.org>.
jayceslesar commented on PR #8558:
URL: https://github.com/apache/iceberg/pull/8558#issuecomment-1720348669
will change this to be more akin to https://github.com/apache/iceberg/pull/8447/files
--
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
[GitHub] [iceberg] jayceslesar commented on pull request #8558: Python: Add `__repr__` for `Table` and `Catalog`
Posted by "jayceslesar (via GitHub)" <gi...@apache.org>.
jayceslesar commented on PR #8558:
URL: https://github.com/apache/iceberg/pull/8558#issuecomment-1718434959
@Fokko Traditionally `__repr__` is supposed to be able to instantiate a class from `exec` or something right? Would it make more sense to attempt to go that route and add the current work in a `__str__` dunder instead? Happy to make those changes if that seems more correct.
--
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
[GitHub] [iceberg] Fokko merged pull request #8558: Draft: Python: Add `__repr__` for `Catalog`
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #8558:
URL: https://github.com/apache/iceberg/pull/8558
--
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
[GitHub] [iceberg] jayceslesar commented on pull request #8558: Draft: Python: Add `__repr__` for `Catalog`
Posted by "jayceslesar (via GitHub)" <gi...@apache.org>.
jayceslesar commented on PR #8558:
URL: https://github.com/apache/iceberg/pull/8558#issuecomment-1731489901
> @jayceslesar This looks great, could you add a test as well?
Done!
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #8558: Draft: Python: Add `__repr__` for `Catalog`
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8558:
URL: https://github.com/apache/iceberg/pull/8558#discussion_r1334539263
##########
python/tests/catalog/test_base.py:
##########
@@ -598,3 +598,8 @@ def test_add_column_with_statement(catalog: InMemoryCatalog) -> None:
schema_id=0,
identifier_field_ids=[],
)
+
+
+def test_catalog_repr(catalog: InMemoryCatalog) -> None:
+ s = repr(catalog)
+ assert s == "test.in.memory.catalog (<class 'test_base.InMemoryCatalog'>)"
Review Comment:
```suggestion
assert s == "test.in.memory.catalog (<class 'test_base.InMemoryCatalog'>)"
```
--
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
[GitHub] [iceberg] jayceslesar commented on a diff in pull request #8558: Draft: Python: Add `__repr__` for `Catalog`
Posted by "jayceslesar (via GitHub)" <gi...@apache.org>.
jayceslesar commented on code in PR #8558:
URL: https://github.com/apache/iceberg/pull/8558#discussion_r1332193770
##########
python/pyiceberg/catalog/__init__.py:
##########
@@ -600,3 +600,7 @@ def _get_updated_props_and_update_summary(
)
return properties_update_summary, updated_properties
+
+ def __repr__(self) -> str:
+ """Returns the string representation of the Catalog class."""
+ return f"{self.__class__} with name: {self.name} and properties: {self.properties}"
Review Comment:
removed that
--
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