Conversation
|
I analyzed the code a bit more. The problem why so many test fail is that currently the library doesn't know the difference between an uninitialized field and a field which is initialized with an empty object. |
| @@ -875,6 +874,10 @@ def parse(self: T, data: bytes) -> T: | |||
| ) | |||
|
|
|||
| current = getattr(self, field_name) | |||
There was a problem hiding this comment.
This should be moved below that if statement
There was a problem hiding this comment.
The line 879 uses the current attribute, so putting that below doesn't make any sense
|
|
||
| current = getattr(self, field_name) | ||
|
|
||
| if self.__raw_get(field_name) == PLACEHOLDER: |
There was a problem hiding this comment.
This should use is not ==
There was a problem hiding this comment.
And maybe there should be a return on this
There was a problem hiding this comment.
Why should there be a return here? The for loop has to continue
| ) | ||
| ): | ||
| output[cased_name] = value.to_dict(casing, include_default_values) | ||
| elif self.__raw_get(field_name) != PLACEHOLDER: |
There was a problem hiding this comment.
Similar thing here, should be is not
There was a problem hiding this comment.
I was thinking about that but I saw line 540 doing the same comparison
|
|
||
| # Serialized after setting something | ||
| foo.bar.baz = 1 | ||
| foo.bar = Bar(baz=1) |
There was a problem hiding this comment.
Changing all this is a regression if this PR breaks this
There was a problem hiding this comment.
I know that isn't great. My PR would break lazy initialization. If you want to keep that, I have to think of another implementation. But lazy initialization also isn't in the google implementation, so I thought it wouldn't be important.
There was a problem hiding this comment.
Well breaking things isn't good. I know how this can be fixed it's been discussed before but it involves every field having a serialized attribute or similar.
|
Hi @dk99, thanks for working on this.
I believe this is by design in order to be in line with proto3 semantics, whereby primitive type fields are interpreted as their zero value if unset, and vice versa. More generally I think the fix for this bug should be isolated to the to_dict structuring logic and shouldn't have any implications for the parsing or internal representation of object. Unless I'm missing something?
As I mentioned in slack channel, as I recall lazy initialisation is required for recursive message types to work, so I suspect the google implementation actually does support it. As for testing, ideally we should have a test case for this in line with the pattern documented here: https://github.com/danielgtaylor/python-betterproto/tree/master/tests Let me know if this doesn't turn out to be trivial (the test setup needs some work which has been done in other pending PRs). |
3 participants
The issue is described here #199.
It was a very simple fix. I don't think a test case is neccesary for this bug. If you think otherwise I can add one.