-
Notifications
You must be signed in to change notification settings - Fork 207
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
Raise AttributeError
on attempts to access unset oneof
fields
#510
Raise AttributeError
on attempts to access unset oneof
fields
#510
Conversation
2b1c650
to
105052c
Compare
I think this PR solves all of the problems mentioned in #358 |
105052c
to
c4b581a
Compare
Thanks for the pr it looks good apart from the changes requiring you to access _Message__unsafe_get which should not be api that anyone should have to deal with. Unless this is just for testing purposes |
_Message__unsafe_get is just for testing/use within betterproto itself, not for the users of betterproto. I don't see any reason why the users of betterproto would want to call _Message__unsafe_get instead of just using |
If it's just for testing like that can you just remove the method and use |
Okay, I will remove it and update the tests to use |
c4b581a
to
774b3c2
Compare
I've removed |
774b3c2
to
ca47cc5
Compare
This commit modifies `Message.__getattribute__` to raise `AttributeError` whenever an attempt is made to access an unset `oneof` field. This provides several benefits over the current approach: * There is no longer any risk of `betterproto` users accidentally relying on values of unset fields. * Pattern matching with `match/case` on messages containing `oneof` groups is now supported. The following is now possible: ``` @dataclasses.dataclass(eq=False, repr=False) class Test(betterproto.Message): x: int = betterproto.int32_field(1, group="g") y: str = betterproto.string_field(2, group="g") match Test(y="text"): case Test(x=v): print("x", v) case Test(y=v): print("y", v) ``` Before this commit the code above would output `x 0` instead of `y text`, but now the output is `y text` as expected. The reason this works is because an `AttributeError` in a `case` pattern does not propagate and instead simply skips the `case`. * We now have a type-checkable way to deconstruct `oneof`. When running `mypy` for the snippet above `v` has type `int` in the first `case` and type `str` in the second `case`. For versions of Python that do not support `match/case` (before 3.10) it is now possbile to use `try/except/else` blocks to achieve the same result: ``` t = Test(y="text") try: v0: int = t.x except AttributeError: v1: str = t.y # `oneof` contains `y` else: pass # `oneof` contains `x` ``` This is a breaking change.
ca47cc5
to
921a2b7
Compare
In future would you mind avoiding force pushing, it makes things harder to review and we squash the commits anyway |
For |
Would you mind also adding a small match test case for the feature you have shown in your OP? |
Okay, I will add it. |
assert betterproto.which_one_of(foo, "group1")[0] == "baz" | ||
|
||
foo.sub.val = 1 |
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.
Is this a behaviour change?
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.
This PR makes it impossible to use foo.sub.val = 1
when foo.sub
is unset in the group.
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.
I think this is a bit of a tradeoff. With this change the users of betterproto
can no longer use the foo.sub.val = 1
syntax for fields that are unset in groups, but this also means that there is less risk of them accidentally changing which field is set in a group.
This is needed to allow formatting Python 3.10 pattern matching syntax.
The pattern matching syntax is only supported in Python 3.10+, so this commit adds it in a separate file to avoid syntax errors for older Python versions.
These is no `23.0.0` release.
Pass `--target-version py310` to allow pattern matching syntax.
I've added a test for pattern matching. Sorry for all the commits, I should've installed |
@Gobot1234 Thank you! Would you mind merging this PR? (since I don't have write access) |
I think it'd be nice to have this reflected in the docs. |
…nielgtaylor#510) # Conflicts: # poetry.lock # src/betterproto/__init__.py # src/betterproto/plugin/parser.py
…nielgtaylor#510) # Conflicts: # poetry.lock # src/betterproto/__init__.py # src/betterproto/plugin/parser.py # Conflicts: # poetry.lock # pyproject.toml
Removed the parts of the example that showed accessing an unset value, as it now raises an `AttributeError`, and added an example of the `match` way of accessing the attributes. Related to danielgtaylor#510 and danielgtaylor#358.
I've opened a PR to change the README to reflect this: |
This commit modifies
Message.__getattribute__
to raiseAttributeError
whenever an attempt is made to access an unsetoneof
field. This provides several benefits over the current approach:There is no longer any risk of
betterproto
users accidentally relying on values of unset fields.Pattern matching with
match/case
on messages containingoneof
groups is now supported. The following is now possible:Before this commit the code above would output
x 0
instead ofy text
, but now the output isy text
as expected. The reason this works is because anAttributeError
in acase
pattern does not propagate and instead simply skips thecase
.We now have a type-checkable way to deconstruct
oneof
. When runningmypy
for the snippet abovev
has typeint
in the firstcase
and typestr
in the secondcase
. For versions of Python that do not supportmatch/case
(before 3.10) it is now possbile to usetry/except/else
blocks to achieve the same result:This is a breaking change. The previous behavior is still accessible via
Message.__unsafe_get
.