python-betterproto icon indicating copy to clipboard operation
python-betterproto copied to clipboard

to_dict on empty nested types

Open kilimnik opened this issue 4 years ago • 2 comments

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.

kilimnik avatar Jan 19 '21 10:01 kilimnik

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.

kilimnik avatar Jan 19 '21 21:01 kilimnik

Hi @DK99, thanks for working on this.

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.

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?

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.

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).

nat-n avatar Jan 24 '21 21:01 nat-n