chore(python): handle duplicate field names#3384
Conversation
|
It seems that dataclass don't allow sublass has duplicate fields with parent class. Could you check whether this is true? If so, the issue can be simplified a lot |
|
I found that dataclass allows a subclass to have duplicate fields with a parent class, but when you define a field with the same name in the subclass, it overrides the parent's field |
|
Does it mean that I needed to only remove the comment of TODO? |
|
Just remove TODO, your code should make it work now |
eyad-hazem-elmorsy
force-pushed
the
chore/handle-duplicate-field-names
branch
from
c725da7 to
cb32dd2
Compare
|
You can check now @chaokunyang |
|
@eyad-hazem-elmorsy Could you add some duplciate fields tests back? THe tests can ensure we don't break it in future |
chaokunyang
changed the title
|
Returned back and the tests run successfully |
| assert field_map["name"].defined_class == "Child", f"Expected field to be defined in Child but got {field_map['name'].defined_class}" | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
I think we also need e2e serialization tests in each case instead of only typedef tests
| assert decoded_field_names.count("name") == 1 | ||
|
|
||
|
|
||
| def test_multilevel_inheritance(): |
There was a problem hiding this comment.
I'm not sure whether such tests are meaningful. The shadow ones shuld already cover it.More code means more maintainance overhead.
Please refactor all tests to to make it consise and remove redundancy
| fory.register(Parent, namespace="test", typename="Parent") | ||
| fory.register(ChildWithShadow, namespace="test", typename="ChildWithShadow") | ||
|
|
||
| obj = ChildWithShadow(name="shadowed", value=10, extra=3.14) |
There was a problem hiding this comment.
Could you add TypeDef encoding check that only threefields for ChildWithShadow?
|
Thanks for your time and these good points. I explored many pieces of code in this PR and learned a lot |
2 participants
Why?
This PR handles the duplicate of field names for inheritance.
What does this PR do?
The handling is done by making every child class's field override its parent's till the most-derived field kept.
First, we iterate over the full class MRO (cls.mro) to extract field metadata from all parent classes.
Then, deduplicate fields so that each field name appears only once in the resulting list with keeping the child class definition if a field is shadowed.
Finally, added some tests to cover:
Does this PR introduce any public API change?
Does this PR introduce any binary protocol compatibility change?