acederberg / pydantic-settings-yaml

A convenient tool for loading pydantic settings from either YAML and JSON.
MIT License
5 stars 3 forks source link

Type Hints for `YamlSettingsConfigDict` are not helpful. #12

Closed acederberg closed 3 months ago

acederberg commented 1 year ago

The YamlSettingsConfigDict class could specify that yaml_reload is not required. Should we use typing.NotRequired or typing.Optional? MyPy is not happy when the type hint is not provided. For instance

    model_config = YamlSettingsConfigDict(
        # yaml_reload=False,
        yaml_files={
            path.realpath(
                path.join(
                    path.dirname(__file__),
                    "pulumi.yaml",
                )
            )
        },
    )

will not satisfy mypy until line 2 is uncommented.

aisven commented 9 months ago

@acederberg Thank you for providing this library.

Is this issue still open, or has it been fixed and released via 2.0.1?

By the way, I think release 2.0.1 is missing its git tag, at least up here at GitHub when looking at https://github.com/acederberg/pydantic-settings-yaml/tags

acederberg commented 9 months ago

@aisven you are correct, the tag is missing entirely. I lost the machine where I made the release and I don't recall the exact commit that release was on.

Also I don't think this is an issue, in my case it was a result of IDE configuration (with regard to type hints not being helpful). The particular use case in the description above does not reproduce the issue elsewhere so I'm not entirely sure what to think. Has it been an issue for you?

acederberg commented 9 months ago

My comment from #15:

Should close #13, #14, and maybe #12

aisven commented 9 months ago

@acederberg No, it has not been an issue for me. So far.

I want to add to the issue description that in order to have something not required, it seems that generally there is a go-to style for the type hint. As far as I am aware, instead of using typing.Optional, a new consensus is to use <the core type hint e.g. str> | None as in "or None" so that the variable can also hold None and is thus optional. This avoids carrying around the Optional as well as importing typing.Optional. However, I am not sure if in case of a dict, typing.NotRequired would be needed instead of the | None thing.

acederberg commented 9 months ago

@aisven I believe in this case the correct way to go is with typing.NotRequired per pep-0655. I should probably update the use of Optional. There are a few cases where using Optional vs | None make a difference, for instance with typer the annotation

ArgUUIDUser: TypeAlias = Annotated[Optional[str], typer.Argument(help="User uuid.")]

will work but using a union will break things (because this library does not yet support the use | None.

It looks like

class YamlFileConfigDict(TypedDict):
    subpath: Optional[str]
    required: bool

could be a problem. After fixing it, the below example

from yaml_settings_pydantic import YamlFileConfigDict

YamlFileConfigDict(
    required=True
)

no longer raises that

No overloads for "__init__" match the provided arguments

I've added this change on fix/py-typed. There is another issue though, and that is that mypy complains about the definition of YamlSettingsConfigDict saying that All bases of a new typeddict must be typeddict types. However the type hints are more helpful when combining TypedDict and ConfigDict. The following clue might help resolve this:

>>> from pydantic import ConfigDict
>>> type(ConfigDict)
<class 'typing_extensions._TypedDictMeta'>
acederberg commented 3 months ago

Update:

This was fixed by changing NotRequired to be on the outermost level.