Support for recursive messages

[chris-chambers]

Changes message initialization (__post_init__) so that default values are no longer eagerly created to prevent infinite recursion when initializing recursive messages.

As a result, PLACEHOLDER will be present in the message for any uninitialized fields. So, an implementation of __get_attribute__ is added that checks for PLACEHOLDER and lazily creates and stores default field values.

And, because PLACEHOLDER values don't compare equal with zero values, a custom implementation of __eq__ is provided, and the code generation template is updated so that messages generate with @dataclass(eq=False, repr=False).

Fixes #13 and fixes #74.

---

This needs careful review! I'm not sure if this is the desired approach. It's just the least-friction way I could think of.

[chris-chambers]

It looks like this may also fix #63. But, since I wasn't trying to do that, it should get some extra review.

[Gobot1234]

There should be a space after this comma, to match PEP8

[Gobot1234]

I think it would be better to insert the part using an f-string and join them using ", " to avoid this.

On another note, since this is for constructing the class from the repr you should probably also check if the field is an Enum and then use Enum.__name__.value.name

[chris-chambers]

I'm not sure I follow this:

On another note, since this is for constructing the class from the repr you should probably also check if the field is an Enum and then use Enum.name.value.name

repr isn't generally meant to be round-trippable. For example:

from dataclasses import dataclass
from enum import Enum

class Color(Enum):
    RED = 1
    BLUE = 2

@dataclass
class Foo(object):
    color: Color

repr(Foo(Color.RED)) # => "Foo(color=<Color.RED: 1>)"

This implementation of __repr__ gives a similar result to what the implementation provided by @dataclass gives.

[Gobot1234]

According to the python docs (https://docs.python.org/3/reference/datamodel.html#object.__repr__) where possible an object's __repr__ should allow for x = eval(repr(x)) where possible, with the current implementation it would not be possible to do this, so I was suggesting that Enum fields should be converted to there Enum class name and member name (the default for str(Enum)) to allow for this.

[chris-chambers]

I agree with your interpretation of the docs, but already the built in enum.Enum does not follow this convention. I'm just delegating to each field's existing __repr__ implementation, which is as much as we should do, in my opinion. I don't think Message's implementation of __repr__ should inspect its field's types and change its behavior.

[chris-chambers]

I just realized that betterproto generates enums that inherit from betterproto.Enum, not enum.Enum. I apologize for my confusion.

Given that, it would be possible to implement betterproto.Enum.__repr__ and get the behavior you're talking about. And it seems like a perfectly good thing to do. It's independent of this PR, though, and would have the same benefit whether it lands before or after this, so I still wouldn't want to tie the two changes together.

[boukeversteegh]

also in favor of keeping general improvements out of scope of bugfix PR's, unless they are essential

[Gobot1234]

I wasn't suggesting altering the betterproto.Enum.__repr__ but altering Message.__repr__ to use enum.Enum.__str__ instead to follow the standard.

[chris-chambers]

Sorry, while these PRs have been in review, I discovered that the encoding and decoding latency of betterproto is too high for my use-case, so I need to put my efforts into another solution.

Please, feel free to make whatever changes you wish, if you want to incorporate these changes. I enabled direct editing of the PR by contributors, so it should be easy to make changes. Or, if you prefer to close this and take another approach to solving this problem, that works for me.

[aleneum]

Hi @chris-chambers,

thank you for your effort nevertheless! I am in need of this feature. Is there something that could be done by me before this PR is ready to be merged (@Gobot1234)?

[Gobot1234]

It LGTM.

[Gobot1234]

I can also fix the conflicts at some point soon-ish

[chris-chambers]

I've got time this afternoon, so I'll rebase and fix the merge conflict. So no need, @Gobot1234

[aleneum]

just fyi: I forked master, rebased and then merged the PR. There was one issue in the template where {% for message in description.messages %} wasnt correctly updated (to output_file) but this seems to be fixed already. Parsing a rather complex message with a redundant item composition no longer caused recursion errors for me.

[nat-n]

Thanks to @chris-chambers et al. for the good work on this. Sorry for the slow turn around.

I pushed some final changes:

• Rebase onto master
• Add more test cases using the standard test pattern
• Tweak some of the new code
• Ensure repr keys are consistently ordered by field number

Ready to merge I'd say 👍