-
-
Notifications
You must be signed in to change notification settings - Fork 97
feat: post_create and post_generate hooks #667
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
base: main
Are you sure you want to change the base?
Conversation
Can the above functionality be achieved by overriding |
It can, but:
|
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.
The above feels like could be handled with docs if wanted to handle that case? Not opposed to adding though depending what other @litestar-org/maintainers think.
polyfactory/factories/base.py
Outdated
:param model: The created model instance. | ||
""" | ||
pass |
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.
Can this return the instance to be consistent with post_generate
?
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.
Yes! I can make that change.
I'm personally not a fan of returning when we don't need to as it hints to the developer that for some reason we need to return and therefore the value is used for something
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.
Do these need to be called on coverage methods too?
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.
Ah, good point. Will add.
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.
Based on this repetition it makes sense to add as need to repeat logic otherwise. Let me know if need anything clarity on other comments!
Documentation preview will be available shortly at https://litestar-org.github.io/polyfactory-docs-preview/667 |
@adhtruong addressed PR feedback! |
Description
Two additional hooks:
I can write some additional tests for this, but wanted to get some initial feedback first.