Use contextvars instead of patching pydantic internals.#44
Use contextvars instead of patching pydantic internals.#44dchukhin merged 1 commit intolincolnloop:mainfrom
Conversation
apollo13
force-pushed
the
pydantic2-fixes
branch
from
47d96c4 to
90a6b30
Compare
| import sys | ||
| from io import StringIO | ||
| from types import GenericAlias | ||
| from typing import ( |
There was a problem hiding this comment.
Most of these where unused, so I removed them, also see the comment on pydantic.fields below.
| from typing import Any, cast, get_args | ||
|
|
||
| from pydantic import PrivateAttr | ||
| from pydantic._internal._config import config_keys |
There was a problem hiding this comment.
This comment belongs to the pydantic.fields import one line below, but github doesn't allow me to set a comment there. I assume Field is ment to be here so from goodconf import Field works. Maybe an extra comment should point that out? Maybe even add an __all__?
goodconf/__init__.py
Outdated
| from pydantic import PrivateAttr | ||
| from pydantic._internal._config import config_keys | ||
| from pydantic.fields import ( # noqa | ||
| Field, |
There was a problem hiding this comment.
I assume Field is ment to be here so from goodconf import Field works. Maybe an extra comment should point that out? Maybe even add an __all__?
There was a problem hiding this comment.
I'm not positive why the from goodconf import Field is there, but I imagine removing it will break something for someone. Both a comment and adding to __all__ sound good to me.
There was a problem hiding this comment.
Yes, the intent was being able ti import it from goodconf
goodconf/__init__.py
Outdated
| return "" | ||
|
|
||
|
|
||
| CONFIG_CONTEXT = ContextVar("_goodconf_config") |
There was a problem hiding this comment.
This might need some consideration whether this is threadsafe or not. I think it might be since a single thread can only call load on one settings class concurrently.
| def load(self, filename: str | None = None) -> None: | ||
| """Find config file and set values""" | ||
| if filename: | ||
| values = _load_config(filename) |
There was a problem hiding this comment.
Even if we are not using my approach with context vars and keep the current approach it might make sense to simply set ._config_file here instead and leave the actual loading to our config source so everything is in one place.
| g = G() | ||
| g.load() | ||
| mocked_load_config.assert_called_once_with(str(path)) | ||
| assert g._config_file == str(path) |
There was a problem hiding this comment.
Removed and similar below because _config_file no longer exists and the assertion doesn't provide much value anyways since we check that load_config is called with that path.
goodconf/__init__.py
Outdated
| from pydantic import PrivateAttr | ||
| from pydantic._internal._config import config_keys | ||
| from pydantic.fields import ( # noqa | ||
| Field, |
There was a problem hiding this comment.
I'm not positive why the from goodconf import Field is there, but I imagine removing it will break something for someone. Both a comment and adding to __all__ sound good to me.
goodconf/__init__.py
Outdated
| return self.load() | ||
| if kwargs or load: | ||
| return self._load(config_file, kwargs) | ||
| else: # Patch `load` to properly pass on the filename |
There was a problem hiding this comment.
Non-critical comment: technically-speaking, the else is not needed here
|
I have another idea that I want to try that might work without contextvars. |
apollo13
force-pushed
the
pydantic2-fixes
branch
2 times, most recently
from
43d1417 to
bdc85f1
Compare
apollo13
force-pushed
the
pydantic2-fixes
branch
from
bdc85f1 to
1e1f7f8
Compare
| "pydantic>=2.0", | ||
| "pydantic-settings>=2.0", | ||
| "pydantic>=2.7", | ||
| "pydantic-settings>=2.4", |
There was a problem hiding this comment.
Needed because accessing the current state requires pydantic-settings>=2.4 which in turn requires pydantic>=2.7 hence I raised that as well.
3 participants
@dchukhin as requested the new PR rebased on main. I'll leave a comment or two inline to explain a few things.