Skip to content

Let default_field_resolver check "typing.Mapping" instead of the more restrictive "dict" #102

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

Closed
jstlaurent opened this issue Aug 7, 2020 · 4 comments

Comments

@jstlaurent
Copy link

Feature requests

The current default_field_resolver checks against instances of dict specifically to decide whether to use get or getattr to access the field on the source. Checking against typing.Mapping would be more flexible, allowing, for example, a ChainMap to be used as the underlying source.

@Cito
Copy link
Member

Cito commented Aug 7, 2020

Sounds like a good idea. But we need to explore these questions:

  • Maybe we should check against collections.abc.Mapping instead? What are the pros/cons?
  • Performance implications: A quick test showed that using collections.abc.Mapping is ~ 6x slower, typing.Mapping is ~ 14x slower than using dict. This may be problematic, since the checks happen so frequently.

@jstlaurent
Copy link
Author

Those are very good questions. Thank you for testing the performance. I wonder if there is a good way to benchmark the impact on the whole process of resolving an object structure.

@Cito
Copy link
Member

Cito commented Aug 8, 2020

It is hardly measurable with our standard benchmarks and probably doesn't matter much in real life as well.

So I've implemented this now and added a test using ChainMap. I've used abc.collections.Mapping because typing.Mapping is intended to be used for type checking, not for runtime checks, and more or less only proxies the checks anyway.

Will be available in the next release.

@Cito Cito closed this as completed Aug 8, 2020
@jstlaurent
Copy link
Author

Thank you very much!

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

No branches or pull requests

2 participants