You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/11/09 12:33:05 UTC
[GitHub] [iceberg] LuigiCerone opened a new pull request, #6159: Python: Update mypy version
LuigiCerone opened a new pull request, #6159:
URL: https://github.com/apache/iceberg/pull/6159
Closes #6148
--
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 #6159: Python: Update mypy version
Posted by GitBox <gi...@apache.org>.
Fokko commented on PR #6159:
URL: https://github.com/apache/iceberg/pull/6159#issuecomment-1308709343
Hey @LuigiCerone thanks for opening this PR! It looks like some of the rules of mypy were updated, and we also need to brush some of the annotations. For example, there is now support for recursive types.
--
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] LuigiCerone commented on a diff in pull request #6159: Python: Update mypy version
Posted by GitBox <gi...@apache.org>.
LuigiCerone commented on code in PR #6159:
URL: https://github.com/apache/iceberg/pull/6159#discussion_r1020777087
##########
python/tests/avro/test_decoder.py:
##########
@@ -106,9 +107,11 @@ def read(self, size: int = 0) -> bytes:
self.pos += 1
return int.to_bytes(1, self.pos, byteorder="little")
+ @abstractmethod
Review Comment:
Ok, fixed as suggested. Thanks! :)
--
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] LuigiCerone commented on a diff in pull request #6159: Python: Update mypy version
Posted by GitBox <gi...@apache.org>.
LuigiCerone commented on code in PR #6159:
URL: https://github.com/apache/iceberg/pull/6159#discussion_r1026091130
##########
python/pyiceberg/utils/config.py:
##########
@@ -44,7 +44,7 @@ def merge_config(lhs: RecursiveDict, rhs: RecursiveDict) -> RecursiveDict:
if isinstance(lhs_value, dict) and isinstance(rhs_value, dict):
# If they are both dicts, then we have to go deeper
new_config[rhs_key] = merge_config(lhs_value, rhs_value)
- else:
+ elif isinstance(lhs_value, str) and isinstance(rhs_value, str):
Review Comment:
Thanks @Fokko :)
Updated as suggested!
--
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 #6159: Python: Update mypy version
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #6159:
URL: https://github.com/apache/iceberg/pull/6159#discussion_r1020961137
##########
python/pyiceberg/catalog/__init__.py:
##########
@@ -120,16 +120,16 @@ def load_catalog(name: str, **properties: Optional[str]) -> Catalog:
or if it could not determine the catalog based on the properties
"""
env = _ENV_CONFIG.get_catalog_config(name)
- conf = merge_config(env or {}, properties)
+ conf = merge_config(env or {}, properties) # type: ignore
Review Comment:
@LuigiCerone I noticed that you ignored the type-check here. Anything that we can do to make this pass instead of suppressing it?
--
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] LuigiCerone commented on a diff in pull request #6159: Python: Update mypy version
Posted by GitBox <gi...@apache.org>.
LuigiCerone commented on code in PR #6159:
URL: https://github.com/apache/iceberg/pull/6159#discussion_r1020523338
##########
python/tests/avro/test_decoder.py:
##########
@@ -106,9 +107,11 @@ def read(self, size: int = 0) -> bytes:
self.pos += 1
return int.to_bytes(1, self.pos, byteorder="little")
+ @abstractmethod
Review Comment:
After the [update of mypy](https://mypy-lang.blogspot.com/2022/11/mypy-0990-released.html), methods with empty bodies are not allowed for type safety. Either we turn the class into an abstract one (but the test will fail for now because it has to be readjusted) or we disable this new functionality with the specific flag (`--allow-empty-bodies`). What do you think?
--
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] LuigiCerone commented on a diff in pull request #6159: Python: Update mypy version
Posted by GitBox <gi...@apache.org>.
LuigiCerone commented on code in PR #6159:
URL: https://github.com/apache/iceberg/pull/6159#discussion_r1023218766
##########
python/pyiceberg/catalog/__init__.py:
##########
@@ -120,16 +120,16 @@ def load_catalog(name: str, **properties: Optional[str]) -> Catalog:
or if it could not determine the catalog based on the properties
"""
env = _ENV_CONFIG.get_catalog_config(name)
- conf = merge_config(env or {}, properties)
+ conf = merge_config(env or {}, properties) # type: ignore
Review Comment:
Yes @Fokko , I'll try to work on 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
[GitHub] [iceberg] LuigiCerone commented on a diff in pull request #6159: Python: Update mypy version
Posted by GitBox <gi...@apache.org>.
LuigiCerone commented on code in PR #6159:
URL: https://github.com/apache/iceberg/pull/6159#discussion_r1020505411
##########
python/pyproject.toml:
##########
@@ -107,7 +107,7 @@ force_grid_wrap = 4
all = true
[tool.mypy]
-no_implicit_optional = true
Review Comment:
@Fokko Yep, you're right. I'll revert that thanks :)
--
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 #6159: Python: Update mypy version
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #6159:
URL: https://github.com/apache/iceberg/pull/6159#discussion_r1025777318
##########
python/pyiceberg/utils/config.py:
##########
@@ -44,7 +44,7 @@ def merge_config(lhs: RecursiveDict, rhs: RecursiveDict) -> RecursiveDict:
if isinstance(lhs_value, dict) and isinstance(rhs_value, dict):
# If they are both dicts, then we have to go deeper
new_config[rhs_key] = merge_config(lhs_value, rhs_value)
- else:
+ elif isinstance(lhs_value, str) and isinstance(rhs_value, str):
Review Comment:
I think we're almost there, but we changed the behavior a bit here. Here we check if both the lhs and rhs are set, but one or both of them can be None. How about:
```
else:
# Take the non-null value, with precedence on rhs
new_config[rhs_key] = lhs_value or rhs_value
```
and then we can remove the `_coalesce` function (it is only used in one place).
--
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] LuigiCerone commented on a diff in pull request #6159: Python: Update mypy version
Posted by GitBox <gi...@apache.org>.
LuigiCerone commented on code in PR #6159:
URL: https://github.com/apache/iceberg/pull/6159#discussion_r1020523338
##########
python/tests/avro/test_decoder.py:
##########
@@ -106,9 +107,11 @@ def read(self, size: int = 0) -> bytes:
self.pos += 1
return int.to_bytes(1, self.pos, byteorder="little")
+ @abstractmethod
Review Comment:
After the [update of mypy](https://mypy-lang.blogspot.com/2022/11/mypy-0990-released.html), methods with empty bodies are not allowed for type safety reasons. Either we turn the class into an abstract one (but the test will fail for now because it has to be readjusted) or we disable this new functionality with the specific flag (`--allow-empty-bodies`). What do you think?
--
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 #6159: Python: Update mypy version
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #6159:
URL: https://github.com/apache/iceberg/pull/6159#discussion_r1020553202
##########
python/tests/avro/test_decoder.py:
##########
@@ -106,9 +107,11 @@ def read(self, size: int = 0) -> bytes:
self.pos += 1
return int.to_bytes(1, self.pos, byteorder="little")
+ @abstractmethod
Review Comment:
Ah, thanks for the explanation. I think it is okay to implement them. For the seek we need to update the `self.pos` and for tell we need to return the pos.
--
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 #6159: Python: Update mypy version
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #6159:
URL: https://github.com/apache/iceberg/pull/6159#discussion_r1019658267
##########
python/pyproject.toml:
##########
@@ -107,7 +107,7 @@ force_grid_wrap = 4
all = true
[tool.mypy]
-no_implicit_optional = true
Review Comment:
@LuigiCerone I think we want to keep this setting to avoid bugs creeping in
--
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 #6159: Python: Update mypy version
Posted by GitBox <gi...@apache.org>.
Fokko merged PR #6159:
URL: https://github.com/apache/iceberg/pull/6159
--
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