-
Notifications
You must be signed in to change notification settings - Fork 32
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: add copy_from method for field assignment #215
Changes from 1 commit
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 |
---|---|---|
|
@@ -436,7 +436,7 @@ def __init__(self, mapping=None, *, ignore_unknown_fields=False, **kwargs): | |
# | ||
# The `wrap` method on the metaclass is the public API for taking | ||
# ownership of the passed in protobuf objet. | ||
mapping = copy.copy(mapping) | ||
mapping = copy.deepcopy(mapping) | ||
if kwargs: | ||
mapping.MergeFrom(self._meta.pb(**kwargs)) | ||
|
||
|
@@ -602,6 +602,35 @@ def __setattr__(self, key, value): | |
if pb_value is not None: | ||
self._pb.MergeFrom(self._meta.pb(**{key: pb_value})) | ||
|
||
def copy_from(self, other): | ||
"""Equivalent for protobuf.Message.CopyFrom | ||
|
||
Args: | ||
other: (Union[dict, ~.Message): | ||
A dictionary or message to reinitialize the values for this message. | ||
""" | ||
if isinstance(other, type(self)): | ||
# Just want the underlying proto. | ||
other = Message.pb(other) | ||
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. This is equivalent to doing 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. Yes, it is equivalent. The code (and documentation) tries to use the accessor instead of touching the attribute directly. 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. 👍🏻 |
||
elif isinstance(other, type(Message.pb(self))): | ||
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. Why does it 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. Because there's no transformation that needs to occur. It's just here to catch the input type and make sure that it doesn't generate the type mismatch exception below. 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. 👍🏻 |
||
# Don't need to do anything. | ||
pass | ||
elif isinstance(other, collections.abc.Mapping): | ||
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 is in case of a repeated field? 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. It's to catch the dictionary syntax: Message.copy_from(message.submessage, {"name": "Lawrence", "armor_class": 15}) 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. Oh good to know that's possible! |
||
# Coerce into a proto | ||
other = type(Message.pb(self))(**other) | ||
else: | ||
raise TypeError( | ||
"invalid argument type to copy to {}: {}".format( | ||
self.__class__.__name__, other.__class__.__name__ | ||
) | ||
) | ||
|
||
# Note: we can't just run self.__init__ because this may be a message field | ||
# for a higher order proto; the memory layout for protos is NOT LIKE the | ||
# python memory model. We cannot rely on just setting things by reference. | ||
# Non-trivial complexity is (partially) hidden by the protobuf runtime. | ||
self._pb.CopyFrom(other) | ||
|
||
|
||
class _MessageInfo: | ||
"""Metadata about a message. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -317,3 +317,27 @@ class Squid(proto.Message): | |
|
||
s = Squid({"mass_kg": 20, "length_cm": 100}, ignore_unknown_fields=True) | ||
assert not hasattr(s, "length_cm") | ||
|
||
|
||
def test_copy_from(): | ||
class Mollusc(proto.Message): | ||
class Squid(proto.Message): | ||
mass_kg = proto.Field(proto.INT32, number=1) | ||
|
||
squid = proto.Field(Squid, number=1) | ||
|
||
m = Mollusc() | ||
s = Mollusc.Squid(mass_kg=20) | ||
Mollusc.Squid.copy_from(m.squid, s) | ||
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 reason we can't have 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. Correct. The general problem is that we have to be very, very careful about adding fields and methods to the message class types because of potential name collisions with fields, and the general solution is to add the method (or field) to the metaclass. This means that method invocation becomes a little more indirect: we invoke it from the class and pass in the instance as the first parameter. 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. 👍🏻 |
||
assert m.squid is not s | ||
assert m.squid == s | ||
|
||
s.mass_kg = 30 | ||
Mollusc.Squid.copy_from(m.squid, Mollusc.Squid.pb(s)) | ||
assert m.squid == s | ||
|
||
Mollusc.Squid.copy_from(m.squid, {"mass_kg": 10}) | ||
assert m.squid.mass_kg == 10 | ||
|
||
with pytest.raises(TypeError): | ||
Mollusc.Squid.copy_from(m.squid, (("mass_kg", 20))) |
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.
addentical
should beidentical
And perhaps
... identical to the field assignment behavior described above.
would help clarify, as "regular assignment" could imply the behavior most would expect outside the context of protobufs.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.
Doh! Good catch. Yes, the proposed wording is more clear.