-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[BEAM-12000] Update deprecated collections to use collections.abc in python sdk #15415
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
94d8c62
93435a0
44fb6a2
ed01936
fdb686e
c2f8a52
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,4 +16,23 @@ | |
| # | ||
|
|
||
| [run] | ||
| omit = target/* | ||
| omit = target/* | ||
|
|
||
| [report] | ||
| exclude_lines = | ||
| # Have to re-enable the standard pragma | ||
| pragma: no cover | ||
| abc.abstractmethod | ||
|
|
||
| # Overload stubs should never be executed. | ||
| @overload | ||
|
|
||
| # Don't complain about missing debug-only code: | ||
| def __repr__ | ||
| if self\.debug | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. The reason I did if self.debug was to make .coveragerc consistent with setup.cfg coverage option. For the infra pr I can make them both self.debug_logging_enabled |
||
|
|
||
| # Don't complain if tests don't hit defensive assertion code: | ||
| raise NotImplementedError | ||
|
|
||
| # Don't complain if non-runnable code isn't run: | ||
| if __name__ == .__main__.: | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,13 +40,13 @@ | |
| @overload | ||
| def pack_Any(msg): | ||
| # type: (message.Message) -> any_pb2.Any | ||
| pass | ||
| ... | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pass vs ... are similar but not same. pass has an inferred return type of None for some type checkers. Pyright will give you an error for using pass here. ... is typical way to mark overloads as stubs.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are test files even type checked? I tried running mypy on this file and got an error. Also discovered mypy.ini for the repo has an invalid value giving this error message,
The mypy error that prevents it from type checking this file is,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting, I think generated _pb2.py files should be excluded from the type checks.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't know about this usage of Ellipsis. Found: https://mypy.readthedocs.io/en/stable/stubs.html and python/typing#109 where this is disussed.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The pb2.py is not type checked, but pb2.pyi is needed for type inference. Looks like proto file has a comment that confuses mypy. I can make a tweak to the comment to make it fine for mypy. |
||
|
|
||
|
|
||
| @overload | ||
| def pack_Any(msg): | ||
| # type: (None) -> None | ||
| pass | ||
| ... | ||
|
|
||
|
|
||
| def pack_Any(msg): | ||
|
|
@@ -65,13 +65,13 @@ def pack_Any(msg): | |
| @overload | ||
| def unpack_Any(any_msg, msg_class): | ||
| # type: (any_pb2.Any, Type[MessageT]) -> MessageT | ||
| pass | ||
| ... | ||
|
|
||
|
|
||
| @overload | ||
| def unpack_Any(any_msg, msg_class): | ||
| # type: (any_pb2.Any, None) -> None | ||
| pass | ||
| ... | ||
|
|
||
|
|
||
| def unpack_Any(any_msg, msg_class): | ||
|
|
@@ -89,13 +89,13 @@ def unpack_Any(any_msg, msg_class): | |
| @overload | ||
| def parse_Bytes(serialized_bytes, msg_class): | ||
| # type: (bytes, Type[MessageT]) -> MessageT | ||
| pass | ||
| ... | ||
|
|
||
|
|
||
| @overload | ||
| def parse_Bytes(serialized_bytes, msg_class): | ||
| # type: (bytes, Union[Type[bytes], None]) -> bytes | ||
| pass | ||
| ... | ||
|
|
||
|
|
||
| def parse_Bytes(serialized_bytes, msg_class): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| import unittest | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RAT check wants you to add a license here, see other files.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you will fix. |
||
| from google.protobuf import timestamp_pb2 | ||
|
|
||
| from apache_beam.utils.proto_utils import pack_Any | ||
| from apache_beam.utils.proto_utils import to_Timestamp | ||
| from apache_beam.utils.proto_utils import unpack_Any | ||
|
|
||
|
|
||
| class ProtoUtilsTest(unittest.TestCase): | ||
| def make_proto_timestamp(self): | ||
| # type: () -> timestamp_pb2.Timestamp | ||
| return to_Timestamp(0) | ||
|
|
||
| def test_none_pack(self): | ||
| packed_none = pack_Any(None) | ||
| assert packed_none is None | ||
|
|
||
| def test_date_pack(self): | ||
| # type: () -> None | ||
| proto_timestamp = self.make_proto_timestamp() | ||
| packed_msg = pack_Any(proto_timestamp) | ||
| orig_msg = unpack_Any(packed_msg, timestamp_pb2.Timestamp) | ||
| none_msg = unpack_Any(packed_msg, None) | ||
| assert proto_timestamp == orig_msg | ||
| assert none_msg is None | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this a left over given that you have a separate test_none_pack?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. test_None_pack is to check packing None. unpack_Any does not allow None for the msg, but allows None for the msg type. So this is intended. I wanted to test both None msg pack and None class unpack. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,9 @@ exclude_lines = | |
| pragma: no cover | ||
| abc.abstractmethod | ||
|
|
||
| # Overload stubs should never be executed. | ||
| @overload | ||
|
|
||
| # Don't complain about missing debug-only code: | ||
| def __repr__ | ||
| if self\.debug | ||
|
|
@@ -53,7 +56,11 @@ exclude_lines = | |
| output = target/site/cobertura/coverage.xml | ||
|
|
||
| [isort] | ||
| line_length = 120 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where does this number come from?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/apache/beam/blob/master/sdks/python/scripts/run_pylint.sh#L105 it's chosen to match isort usage in CI.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Came across https://issues.apache.org/jira/browse/BEAM-3745 with touches on isort & lint. perhaps we can close it already or after your changes. |
||
| known_standard_library = dataclasses | ||
| force_single_line = True | ||
| combine_star = True | ||
| order_by_type = True | ||
|
|
||
| [yapf] | ||
| indent_width = 2 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added as CI runs isort but pre-commit doesn't. Should have same behavior as ci. The setup.cfg tweaks I did were to match run_pylint.sh in CI.