-
-
Notifications
You must be signed in to change notification settings - Fork 78
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
fix: include pydantic Field constraints when using Optional type #323
fix: include pydantic Field constraints when using Optional type #323
Conversation
polyfactory/utils/predicates.py
Outdated
@@ -79,7 +79,7 @@ def is_union(annotation: Any) -> "TypeGuard[Any | Any]": | |||
|
|||
|
|||
def is_optional_union(annotation: Any) -> "TypeGuard[Any | None]": | |||
"""Determine whether a given annotation is 'typing.Optional'. | |||
"""Determine whether a given annotation is 'typing.Union[XXXX, NoneType]'. |
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.
Feel free to disagree, but I would rename this function to is_optional
, I got too confused with the _union
part. I modified the docstring to reflect that Optional[XXX]
is the same as Union[XXX, NoneType]
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.
You can rename this
I don't know why, but when being run from I tried debugging here below, but in the end, I simply skipped pre-commit for the for this fix. Running it by specifying target versions does seem to show the correct errors.
The python
and from
after modifying the hook like so: - repo: https://github.com/charliermarsh/ruff-pre-commit
rev: "v0.0.280"
hooks:
- id: ruff
args: ["--show-settings"] but somehow |
203a534
to
6154cf4
Compare
install: | ||
poetry install --extras full --with docs --no-ansi --sync | ||
pre-commit install | ||
.PHONY: install |
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.
One command install for newcomers. Is the default when running make
without arguments.
The CI is not coherent regarding Maybe it's because |
There is a |
Gotcha, thank you. Removing it fixes all the shenanigans. |
Apologies if I was unclear, my last comment was not a suggestion to remove aforementioned import, it was just to let you know what it did. That should not be removed. |
Are you sure? The code is written with Python 3.8 syntax, which is the earliest version we support, and it makes the CI pass, I think we should be able to remove it. Moreover, these are test files, so we can be a bit more lenient. |
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.
Looks good. Let us know when you're done and we can merge
@all-contributors add @tcrasset code |
I've put up a pull request to add @tcrasset! 🎉 |
@Goldziher I'm done. Could you release a new version once merged? |
@JacobCoffee and @provinzkraut - can you guys update the dependencies and release a patch version? I would rather you guys take this over. |
Hello! 👋
I had initially commented here, but this was a separate issue. So I just went ahead and fixed it.
Pull Request Checklist
Description