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/05/09 06:35:05 UTC
[GitHub] [iceberg] Fokko opened a new pull request, #4728: Remove mypy error
Fokko opened a new pull request, #4728:
URL: https://github.com/apache/iceberg/pull/4728
Mypy complaints about the config file:
```
No [mypy] section in config file
```
Also enabled some additional checks on unreachable code
--
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] rdblue commented on a diff in pull request #4728: Python: Remove mypy error
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4728:
URL: https://github.com/apache/iceberg/pull/4728#discussion_r868746492
##########
python/src/iceberg/transforms.py:
##########
@@ -83,10 +83,8 @@ def preserves_order(self) -> bool:
def satisfies_order_of(self, other) -> bool:
return self == other
- def to_human_string(self, value) -> str:
- if value is None:
- return "null"
- return str(value)
+ def to_human_string(self, value: Optional[S]) -> str:
+ return str(value) if value is not None else "null"
Review Comment:
Is this change necessary for this PR? We generally try to avoid code churn that isn't making functional changes.
--
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 #4728: Python: Remove mypy error
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #4728:
URL: https://github.com/apache/iceberg/pull/4728#discussion_r867723268
##########
python/src/iceberg/transforms.py:
##########
@@ -120,9 +120,6 @@ def hash(self, value: S) -> int:
raise NotImplementedError()
def apply(self, value: S) -> Optional[int]:
- if value is None:
Review Comment:
Okay, looking at the tests, we _might_ expect a `None` there. I've changed the code the other way around.
--
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] rdblue merged pull request #4728: Python: Remove mypy error
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #4728:
URL: https://github.com/apache/iceberg/pull/4728
--
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 #4728: Remove mypy error
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #4728:
URL: https://github.com/apache/iceberg/pull/4728#discussion_r867672939
##########
python/src/iceberg/transforms.py:
##########
@@ -120,9 +120,6 @@ def hash(self, value: S) -> int:
raise NotImplementedError()
def apply(self, value: S) -> Optional[int]:
- if value is None:
Review Comment:
Since we have `S`, it can never be `None`, otherwise, the signature should be `Optional[S]`. If we want, we can still check if it `None`, `""` or `[]` using:
```python
if not value:
```
I've removed it for now, because according to the current definition it is unreachable code.
--
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 #4728: Remove mypy error
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #4728:
URL: https://github.com/apache/iceberg/pull/4728#discussion_r867672939
##########
python/src/iceberg/transforms.py:
##########
@@ -120,9 +120,6 @@ def hash(self, value: S) -> int:
raise NotImplementedError()
def apply(self, value: S) -> Optional[int]:
- if value is None:
Review Comment:
Since we have `S`, it can never be `None`, otherwise, the signature should be `Optional[S]`. If we want, we can still check if it `None`, `""` or `[]` using:
```python
if not value:
```
--
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] rdblue commented on a diff in pull request #4728: Python: Remove mypy error
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4728:
URL: https://github.com/apache/iceberg/pull/4728#discussion_r868747159
##########
python/src/iceberg/transforms.py:
##########
@@ -120,9 +120,6 @@ def hash(self, value: S) -> int:
raise NotImplementedError()
def apply(self, value: S) -> Optional[int]:
- if value is None:
Review Comment:
Yeah, transforms can be applied to optional columns so this should be `Optional[S]`.
--
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] rdblue commented on pull request #4728: Python: Remove mypy error
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4728:
URL: https://github.com/apache/iceberg/pull/4728#issuecomment-1121793259
Thanks, @Fokko!
--
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