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

feat: pass on factory config #483

Merged
merged 2 commits into from
Jan 18, 2024

Conversation

adhtruong
Copy link
Collaborator

Pull Request Checklist

  • New code has 100% test coverage
  • (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
  • (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR
  • Pre-Commit Checks were ran and passed
  • Tests were ran and passed

Description

Close Issue(s)

@adhtruong adhtruong requested review from a team as code owners January 16, 2024 12:06
@adhtruong adhtruong force-pushed the allow-factory-config-inheritance branch 5 times, most recently from 6394ee7 to 92307f3 Compare January 16, 2024 12:15
@adhtruong adhtruong marked this pull request as draft January 16, 2024 12:18
@adhtruong adhtruong force-pushed the allow-factory-config-inheritance branch 2 times, most recently from d065e9d to 6ecf248 Compare January 16, 2024 12:33
@guacs
Copy link
Member

guacs commented Jan 16, 2024

This is really nice! I had considered this approach a while back, but I faced an issue with how to pass the provider map. In the current test case, there wouldn't be any issues since both the parent and child are both dataclasses. The issue becomes when the parent and child are of different base classes. For example, consider a parent class being a pydantic model and a child class being a msgspec Struct. In this case, the child factory (i.e. the factory for the msgspec struct) would get the provider map of the parent pydantic model. However, the default MsgspecFactory overrides the default get_provider_map to inject types specific to msgspec.

The simplest option would be to tell users to not mix these models. It's fine if they mix pydantic with the Python builtins like dataclass, or typeddict since we don't have any special cases to handle there and, so, we don't override the get_provider_map. Maybe we could raise a warning if users are mixing it like that. I think, from a practical standpoint, this is a fine solution since it would probably be rare for them to be mixing something like pydantic and msgspec.

EDIT: Completely disregard what I said. I just realized you're using __extra_provider_map__ to handle this case. With your approach, the issue I mentioned above is not an issue anymore.

@adhtruong adhtruong force-pushed the allow-factory-config-inheritance branch from 6ecf248 to fa34a6f Compare January 16, 2024 12:40
@adhtruong adhtruong marked this pull request as ready for review January 16, 2024 12:43
Copy link
Member

@guacs guacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really great @adhtruong! Thanks a bunch for this :)

Could you add the details regarding this in the docs? This way, if users add custom fields that they want to pass to the child factories, they'll know to override the config keys field.

polyfactory/factories/base.py Outdated Show resolved Hide resolved
@adhtruong adhtruong force-pushed the allow-factory-config-inheritance branch from fa34a6f to b0b4601 Compare January 17, 2024 17:18
@adhtruong adhtruong force-pushed the allow-factory-config-inheritance branch from b0b4601 to 1bbeecc Compare January 17, 2024 17:21
@adhtruong
Copy link
Collaborator Author

Thanks @guacs for the review! Updated now :)

Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/polyfactory-docs-preview/483

@guacs guacs merged commit c7556e8 into litestar-org:main Jan 18, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants