Skip to content

Conversation

smessmer
Copy link
Contributor

This allows usage of the google.protobuf type stubs from python 3 code.

@JelleZijlstra
Copy link
Member

Thanks for the PR! What have you done to test that these stubs work right in Python 3?

@smessmer
Copy link
Contributor Author

We're using these in one of our projects, where they work fine. However, coverage is very likely incomplete.

@JelleZijlstra
Copy link
Member

Thanks. I noticed there are a ton of functions like protobuf/wrappers_pb2.pyi: def FromString(cls, s: str) -> FloatValue: .... Do those really take str, not bytes in Python 3? I don't have an easy way to check.

@smessmer
Copy link
Contributor Author

They probably take bytes, but I don't have an easy way to check either. I did change a few call sites from str to bytes where we used them and I know they're actually meant to be bytes.

@JelleZijlstra
Copy link
Member

Is it reasonable to assume that all FromString methods take the same type?

odahoda added a commit to odahoda/noisicaa that referenced this pull request May 19, 2018
- Using my own git fork, because I had to make some tweaks.
- Also adding stubs for google.protobuf to my own typeshed. mypy currently has the stubs only for
  python2, until python/typeshed#2140 gets fixed and included in the pip
  version of mypy.
- Also fix some of the issues uncovered by mypy now.
@smessmer
Copy link
Contributor Author

smessmer commented May 22, 2018

I'm unsure. Somebody implemented MergeFromString and ParseFromString in Message to take type Any.

@gvanrossum
Copy link
Member

gvanrossum commented May 23, 2018 via email

@JelleZijlstra
Copy link
Member

This has some conflicts now.

@nipunn1313
Copy link
Contributor

All FromString methods do in fact take bytes. They take in a serialized input.
FromString is a bit of a misnomer in python3, but the naming was designed for python2.

I believe that google.protobuf uses the same code for both python2 and python3, so as long as we use bytes and Text, we shouldn't have (in theory) any typing issues.

That being said, unexpected things do occur. I'm not sure there's a worthwhile way for us to derisk all of these up front.

In #2174 - I made a similar diff which replaced all instances of str -> bytes.

@JelleZijlstra
Copy link
Member

I merged #2174 instead.

@smessmer smessmer deleted the protobuf branch June 1, 2018 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants