Skip to content
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

feat: added ParseFromString method to message for compatibility #336

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

simon-saliba
Copy link

@simon-saliba simon-saliba commented Feb 12, 2022

This PR adds the ParseFromString attribute to the message class for compatibility with the official Google protobuf implementation and method exposure.
Closes #323

@simon-saliba simon-saliba force-pushed the add-ParseFromString-method-to-message branch from d0b5cd2 to cb9161b Compare February 12, 2022 17:23
@Laurens-T
Copy link

Thanks!

@@ -1022,6 +1022,29 @@ def FromString(cls: Type[T], data: bytes) -> T:
"""
return cls().parse(data)

# For compatibility with Google protobuf official implementation.
def ParseFromString(self, data: bytes) -> T:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def ParseFromString(self, data: bytes) -> T:
def ParseFromString(self: T, data: bytes) -> T:

:class:`Message`
The initialized message.
"""
return self.parse(data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this slightly different from the offical implementation which clears any currently set values? https://github.com/protocolbuffers/protobuf/blob/2a2a9b6e64e0cda49b0e5377cad58c790087e1c9/python/google/protobuf/message.py#L193-L199

Choose a reason for hiding this comment

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

This indeed seems to be the case.

I've added a simple example which shows the difference in result from ParseFromString with the one from this PR:

syntax = "proto3";

package some.package.name;

message Cat {
  int64 age = 1;
  int64 lives = 2;
}
def ParseFromString(self, data: bytes):
    return self.parse(data)

if __name__ == '__main__':
    # Create bytes
    cat = Cat(
        age=5,
        # lives=9,
    )

    data = cat.SerializeToString()

    # Dynamically add ParseFromString to the Cat class
    Cat.ParseFromString = ParseFromString

    # ParseFromString for the betterproto cat and the pb2 cat
    cat_betterproto = Cat(age=3, lives=8)
    cat_pb2 = Cat_pb2(age=3, lives=8)

    cat_betterproto.ParseFromString(data)
    cat_pb2.ParseFromString(data)

    print(cat_betterproto)  # Cat(age=5, lives=8), lives are not cleared
    print(cat_pb2)  # age: 5, lives are cleared
    assert cat_pb2.lives == 0

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.

Add ParseFromString for compatibility
4 participants