Skip to content

Makes DESCRIPTOR non nullable again #7125

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 4, 2022
Merged

Makes DESCRIPTOR non nullable again #7125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1301aa8
Makes `DESCRIPTOR` non nullable again
asyks Feb 3, 2022
2e57b77
Add comment to allowlist
asyks Feb 4, 2022
7026010
Spelling/capitalization fixes to stubs/protobuf/@tests/stubtest_allow…
asyks Feb 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions stubs/protobuf/@tests/stubtest_allowlist.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@
google.protobuf\..*_pb2\..*

google.protobuf.internal.containers.BaseContainer.sort
# While Message and Descriptor are both defined with a null DESCRIPTOR,
# subclasses of Message and instances of EnumTypeWrapper require this value to
# be set, and since these type stubs are intended for use with protoc-generated
# python it's more accurate to make them non-nullable.
google.protobuf.internal.enum_type_wrapper.EnumTypeWrapper.DESCRIPTOR
google.protobuf.message.Message.DESCRIPTOR
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would appreciate a short comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Akuli - I'm new to mypy stubtests but I'm seeing the following locally (linux 5.13.0-28-generic, python 3.9.5).

Without these two allowlist entries:

python tests/stubtest_third_party.py protobuf
...
error: google.protobuf.internal.enum_type_wrapper.EnumTypeWrapper.DESCRIPTOR variable differs from runtime type None
Stub: at line 10
google.protobuf.descriptor.EnumDescriptor
Runtime:
None

error: google.protobuf.message.Message.DESCRIPTOR variable differs from runtime type None
Stub: at line 14
google.protobuf.descriptor.Descriptor
Runtime:
None

stubtest failed for protobuf

With these two allowlist entries:

python tests/stubtest_third_party.py protobuf
...
stubtest succeeded for protobuf

My interpretation of that is: while Message and Descriptor are both defined with a null DESCRIPTOR, sublcasses of Message and instances of EnumTypeWrapper require this value to be set, and since These type stubs are intended for use with protoc generated python it's more accurate to make them non-nullable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while Message and Descriptor are both defined with a null DESCRIPTOR, sublcasses of Message and instances of EnumTypeWrapper require this value to be set, and since These type stubs are intended for use with protoc generated python it's more accurate to make them non-nullable.

Add a comment that says this to the allowlist :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! Your understanding is perfect.

Just need a comment in the allowlist.txt file so that someone in the future doesn't revert your change by accident.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, got it, done.

google.protobuf.message.Message.SerializePartialToString
google.protobuf.message.Message.SerializeToString
google.protobuf.service.Service.GetDescriptor
Expand Down
2 changes: 1 addition & 1 deletion stubs/protobuf/google/protobuf/internal/enum_type_wrapper.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ _V = TypeVar("_V", bound=int)
# Expose a generic version so that those using mypy-protobuf
# can get autogenerated NewType wrapper around the int values
class _EnumTypeWrapper(Generic[_V]):
DESCRIPTOR: EnumDescriptor | None
DESCRIPTOR: EnumDescriptor
def __init__(self, enum_type: EnumDescriptor) -> None: ...
def Name(self, number: _V) -> str: ...
def Value(self, name: Text | bytes) -> _V: ...
Expand Down
2 changes: 1 addition & 1 deletion stubs/protobuf/google/protobuf/message.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class EncodeError(Error): ...
_M = TypeVar("_M", bound=Message) # message type (of self)

class Message:
DESCRIPTOR: Descriptor | None
DESCRIPTOR: Descriptor
def __deepcopy__(self, memo=...): ...
def __eq__(self, other_msg): ...
def __ne__(self, other_msg): ...
Expand Down