fix: include pydantic Field constraints when using Optional type

[tcrasset]

Hello! 👋

I had initially commented here, but this was a separate issue. So I just went ahead and fixed it.

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

• Fix pydantic metadata parsing on optional field

[tcrasset]

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]

[Goldziher]

You can rename this

[tcrasset]

I don't know why, but ruff insists on formatting Optional[Exception to Exception | None, (which is a Python 3.10 feature, and the default target_version for ruff is py310.)

when being run from pre-commit and manually from the command line without specifying configuration values. It seems it can't import them from pyproject.toml and uses preconfigured defaults.

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.

❯ poetry run ruff tests/test_pydantic_factory.py --target-version py38 --show-fixes 
❯ poetry run ruff tests/test_pydantic_factory.py --target-version py310 --show-fixes

tests/test_pydantic_factory.py:34:16: UP007 [*] Use `X | Y` for type annotations
Found 2 errors.
[*] 2 potentially fixable with the --fix option.

The python target_version in pyproject.toml is set to py38 and running ruff manually from the command shows that does take it into account:

❯ poetry run ruff tests/test_pydantic_factory.py --show-settings | grep target
    target_version: Py38,

and from pre-commit too

❯ pre-commit run --all-files --verbose | grep target
    target_version: Py38,

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 poetry run ruff --fix does not and defaults to py310.

[tcrasset]

One command install for newcomers. Is the default when running make without arguments.

[tcrasset]

The CI is not coherent regarding ruff and Optional[X] vs X | None.
The validate workflow runs pre-commit which expect the latter, while the other tests expect the former.

Maybe it's because pre-commit is installed with python 3.11?

[Alc-Alc]

The CI is not coherent regarding ruff and Optional[X] vs X | None. The validate workflow runs pre-commit which expect the latter, while the other tests expect the former.

Maybe it's because pre-commit is installed with python 3.11?

There is a from __future__ import annotations import at the top of the file, that will ensure this syntax (type | None) works on certain older python versions as well

[tcrasset]

Gotcha, thank you. Removing it fixes all the shenanigans.

[Alc-Alc]

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.

[tcrasset]

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.

[Goldziher]

Looks good. Let us know when you're done and we can merge

[Goldziher]

@all-contributors add @tcrasset code

[allcontributors]

@Goldziher

I've put up a pull request to add @tcrasset! 🎉

[tcrasset]

@Goldziher I'm done. Could you release a new version once merged?

[Goldziher]

@JacobCoffee and @provinzkraut - can you guys update the dependencies and release a patch version? I would rather you guys take this over.