Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matt035343
Copy link
Collaborator

Fixes #270

Fixing case where multiple fields have the same field name by inverting the map holding the field name overrides

…ct representation by inverting the name override mapping
@github-actions
Copy link

github-actions bot commented Aug 12, 2023

Coverage

Coverage Report
FileStmtsMissCoverMissing
dataclasses_json
   cfg.py51492%80, 84–86
   core.py2381096%38–41, 51, 64, 66, 81, 83, 172, 200
   mm.py2023085%33–36, 42–45, 53–56, 62–65, 88, 161–162, 167, 171, 175, 180, 184, 188, 196, 202, 207, 216, 221, 226, 235, 244–251
   stringcase.py25388%59, 76, 97
   undefined.py143299%24, 38
   utils.py1283672%11–24, 44–49, 60–64, 74, 99–100, 108–109, 124–132, 158, 177, 202
tests
   entities.py226299%229, 235
   test_annotations.py814248%50–67, 78–102, 106–122
   test_api.py142497%88, 99, 139–140
   test_str_subclass.py22195%9
   test_union.py981090%87–94, 108–115
TOTAL250214494% 

Tests Skipped Failures Errors Time
293 3 💤 0 ❌ 0 🔥 6.945s ⏱️

Comment on lines +43 to +46
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"))
Copy link
Collaborator

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?

Copy link
Collaborator Author

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?

Copy link
Collaborator

@george-zubrienko george-zubrienko Aug 13, 2023

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', '')

Copy link
Collaborator

@george-zubrienko george-zubrienko Aug 13, 2023

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.

Copy link
Collaborator Author

@matt035343 matt035343 Aug 14, 2023

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

Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use the same field_name in more than one field
2 participants