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.

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

Add explicit type hint to BaseSettings.model_config #97

Merged
merged 1 commit into from
Jul 4, 2023

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented Jul 3, 2023

Noticed this small issue when working inside settings_customise_sources. Before this PR (by using either cls or settings_cls):

image

After:

image

What's weird is that my IDE seems to infer the type correctly in other places, even without this improvement:

image

By the way, is the settings_cls argument necessary, as settings_customise_sources is already a class method and we have access to cls?

Selected Reviewer: @dmontagu

@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (107e8de) 97.42% compared to head (434bca1) 97.42%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #97   +/-   ##
=======================================
  Coverage   97.42%   97.42%           
=======================================
  Files           5        5           
  Lines         311      311           
  Branches       68       68           
=======================================
  Hits          303      303           
  Misses          6        6           
  Partials        2        2           
Impacted Files Coverage Δ
pydantic_settings/main.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Viicos
Copy link
Contributor Author

Viicos commented Jul 3, 2023

I think the mypy issue comes from the fact that model_config is defined in two places on BaseModel:

    if typing.TYPE_CHECKING:
        # Here we provide annotations for the attributes of BaseModel.
        # Many of these are populated by the metaclass, which is why this section is in a `TYPE_CHECKING` block.
        # However, for the sake of easy review, we have included type annotations of all class and instance attributes
        # of `BaseModel` here:

        # Class attributes
        model_config: ClassVar[ConfigDict]

And later on:

    __slots__ = '__dict__', '__pydantic_fields_set__', '__pydantic_extra__', '__pydantic_private__'

    model_config = ConfigDict()
    __pydantic_complete__ = False
    __pydantic_root_model__ = False

Would it be safe to remove the first one in the if TYPE_CHECKING block, and modify the remaining one to model_config: ClassVar[ConfigDict] = ConfigDict()?

@hramezani
Copy link
Member

Thanks @Viicos for this patch.

Yeah, seems the mypy error will be fixed by the change you mentioned on pydantic side.

You have to create a PR on pydantic and suggest the change. We(other people on the team and I) will review the PR. they may have different opinions on that.

@davidhewitt
Copy link
Contributor

I noticed the lint is using pydantic 2.0b3 and since beta 3 we had pydantic/pydantic#6260 which added the type annotation to model_config which is proposed to be changed in pydantic/pydantic#6393

Perhaps there is no change needed in pydantic and the mypy issue will be resolved by updating the linting requirements to 2.0? We could merge #99 and then rebase this PR to confirm.

@Viicos
Copy link
Contributor Author

Viicos commented Jul 3, 2023

@davidhewitt Good catch, I will rebase once #99 gets merged. It seems to work with a MRE:

https://mypy-play.net/?mypy=latest&python=3.11&gist=ba7caebef09f0b3aa5397d05dfef4541

(the error appears if you remove the if TYPE_CHECKING block).

@hramezani
Copy link
Member

@Viicos As #99 has been merged, Could you please rebase this PR on top of main?

@Viicos
Copy link
Contributor Author

Viicos commented Jul 4, 2023

Rebasing fixed it.

Please review

@hramezani
Copy link
Member

Thanks @Viicos

@hramezani hramezani merged commit 474ed0a into pydantic:main Jul 4, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants