-
Notifications
You must be signed in to change notification settings - Fork 154
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
Fixing case where multiple fields have the same field name #469
base: master
Are you sure you want to change the base?
Conversation
…ct representation by inverting the name override mapping
Coverage Report
|
class MyDataClass: | ||
first: str = field(metadata=config(field_name="myJsonField", decoder=lambda x: x["first"])) | ||
second: str = field(metadata=config(field_name="myJsonField", decoder=lambda x: x["second"])) | ||
other_field: str = field(metadata=config(field_name="otherField")) |
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'm not sure, but is this a good test case? Like, declaring myJsonField
as a regular dict will not trigger the fixed behaviour first place?
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.
@george-zubrienko I am not sure I understand. Are you suggesting to use a dataclass instead of a dict?
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 mean the MyDataClass
can be defined like this:
class MyDataClass:
myJsonField: dict
other_field: str = field(metadata=config(field_name="otherField"))
@property
def first(self) -> str:
return self.myJsonField.get('first', '')
@property
def second(self) -> str:
return self.myJsonField.get('second', '')
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.
Just IMO, the way the test case is defined, dataclass does not reflect the actual data structure being read, but rather relies on some rather artificial deserializing behaviour, which for example in v1 warrants a custom serializer for the class. If this is something people actually do in their user code, I'd like to see examples. I am near 100% certain we will not support these kind of cases in v1 and force people to write custom serializer, thus I'd be wary of using these as test cases for v0 as well.
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.
@george-zubrienko This is PR spawned from #270. Indeed, it is a funky usecase, however, the core here is that two dataclass-json field names maps from the same encoded JSON field name
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.
@george-zubrienko and I discussed that it is actually a bad practice do like #270 does. It will only work one way (decoding only, not encoding). Instead, use the approach @george-zubrienko outlined above.
I will discontinue this PR and make a new one that throws an exception instead.
Fixes #270
Fixing case where multiple fields have the same field name by inverting the map holding the field name overrides