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

Check for collections.abc.Mapping instead of dict when converting a… #3101

Closed
wants to merge 1 commit into from

Conversation

qhefb
Copy link

@qhefb qhefb commented Aug 15, 2021

… pydantic field to a dict.

Change Summary

We would like to be able use a custom immutablefrozendict in fields as pydantic and have dict() continue to work as expected. So we should use a more generic type like collections.abc.Mapping instead of dict to make sure that we still convert it to a dict.

Related issue number

N/A

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

please review

facebook-github-bot pushed a commit to facebookincubator/antlir that referenced this pull request Aug 19, 2021
…of dict.

Summary:
We want to add a `frozendict` class to `shape`, which is a pydantic model. However, when pydantic gets the attributes for a model for serialization, it only makes copies of dict objects, not dict-like objects like `Mapping`. We should treat these as dicts and make dict-like copies so we update the check to `collections.abc.Mapping`.

We can revert this diff once the public facing pull request pydantic/pydantic#3101 is merged upstream, and our depedency is updated.

Reviewed By: snarkmaster

Differential Revision: D30378486

fbshipit-source-id: 185fc483d594f0ecf1d2fcaca39e7d719b936a7c
@qhefb
Copy link
Author

qhefb commented Aug 20, 2021

please review

@JacobHayes
Copy link
Contributor

Any chance this (partially) fixes #3122 (frozendict[k, v] being converted to a dict)?

@PrettyWood
Copy link
Member

If I have

    class Model(BaseModel):
        a: MyCustomMap[str, int]

I expect my value in the dict representation to be a instance of MyCustomMap, not a dict.
Please open an issue before to talk about this

@PrettyWood PrettyWood closed this Sep 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants