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.

Sign up for GitHub

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

Jump to bottom

feat: add recursion guard #468

Merged
merged 13 commits into from
Jan 16, 2024
Merged

feat: add recursion guard #468

merged 13 commits into from
Jan 16, 2024

Conversation

adhtruong
Copy link
Collaborator

@adhtruong adhtruong commented Jan 2, 2024 •
edited
Loading

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

  • Add new new arg build_context to build related methods
  • Track seen models in this parameter to guard against recursion
  • Note this may arguably be backwards breaking but would expect recursion errors so fine to add as a default. Happy to add a configuration instead if preferable

Close Issue(s)

@adhtruong adhtruong requested review from a team as code owners January 2, 2024 11:03
@sourcery-ai sourcery-ai bot mentioned this pull request Jan 2, 2024
Closed
adhtruong
adhtruong commented Jan 2, 2024
View reviewed changes
polyfactory/factories/base.py Outdated Show resolved Hide resolved
polyfactory/factories/base.py Show resolved Hide resolved
polyfactory/factories/base.py Show resolved Hide resolved
guacs
guacs reviewed Jan 3, 2024
View reviewed changes
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.

Thanks @adhtruong! I like the use of the BuildContext.

polyfactory/factories/base.py Outdated Show resolved Hide resolved
polyfactory/factories/pydantic_factory.py Outdated Show resolved Hide resolved
tests/test_passing_build_args_to_child_factories.py Outdated Show resolved Hide resolved
@guacs
Copy link
Member

guacs commented Jan 3, 2024

Note this may arguably be backwards breaking but would expect recursion errors so fine to add as a default

I agree with you. I don't think adding the recursion guard is a breaking change. It doesn't change the behavior nor the returned type.

adhtruong
adhtruong commented Jan 3, 2024
View reviewed changes
tests/test_passing_build_args_to_child_factories.py Outdated Show resolved Hide resolved
polyfactory/factories/pydantic_factory.py Outdated Show resolved Hide resolved
polyfactory/factories/base.py Outdated Show resolved Hide resolved
polyfactory/utils/helpers.py Show resolved Hide resolved
@adhtruong adhtruong force-pushed the add-recursion-guard branch from 9329754 to 66475db Compare January 3, 2024 14:10
@adhtruong adhtruong force-pushed the add-recursion-guard branch from 56fd574 to a22a980 Compare January 3, 2024 14:48
guacs
guacs reviewed Jan 5, 2024
View reviewed changes
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 looks great! I just had a few doubts/comments. Once those are cleared up, I think this is good to go.

docs/examples/library_factories/sqlalchemy_factory/test_example_4.py Outdated Show resolved Hide resolved
polyfactory/factories/base.py Show resolved Hide resolved
polyfactory/factories/base.py Outdated Show resolved Hide resolved
tests/test_passing_build_args_to_child_factories.py Outdated Show resolved Hide resolved
@adhtruong
Copy link
Collaborator Author

All comments addressed now:

  1. Implementation of coverage updated
  2. Type errors resolved

@guacs
Copy link
Member

guacs commented Jan 11, 2024

@adhtruong that's fantastic! I'll take a look at it today or tomorrow :)

guacs
guacs reviewed Jan 14, 2024
View reviewed changes
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.

If we can somehow workaround the type checkers as well, then I think it'd be great. I've suggested a possible workaround. Let me know what you think @adhtruong.

docs/examples/library_factories/sqlalchemy_factory/test_example_4.py Outdated Show resolved Hide resolved
polyfactory/utils/helpers.py Show resolved Hide resolved
tests/test_passing_build_args_to_child_factories.py Outdated Show resolved Hide resolved
@adhtruong adhtruong force-pushed the add-recursion-guard branch from 44ab851 to b444845 Compare January 15, 2024 11:32
guacs
guacs approved these changes Jan 16, 2024
View reviewed changes
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.

Thanks @adhtruong!

@guacs guacs enabled auto-merge (squash) January 16, 2024 07:07
@guacs guacs merged commit 80bd012 into litestar-org:main Jan 16, 2024
18 of 19 checks passed
@github-actions GitHub Actions
Copy link

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

@adhtruong adhtruong mentioned this pull request Jan 16, 2024
Closed
@adhtruong adhtruong deleted the add-recursion-guard branch January 17, 2024 13:56
This was referenced Jan 19, 2024
Closed
Closed
Closed
Closed
@adhtruong adhtruong mentioned this pull request Mar 10, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guacs guacs guacs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for relationship building models in SQLAlchemyFactory
2 participants
@adhtruong @guacs