-
-
Notifications
You must be signed in to change notification settings - Fork 43
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 default value to source classes init params #82
Conversation
please review |
pydantic_settings/sources.py
Outdated
@@ -22,6 +22,7 @@ | |||
|
|||
|
|||
DotenvType = Union[Path, List[Path], Tuple[Path, ...]] | |||
env_file_sentinel: DotenvType = Path('') |
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.
env_file_sentinel: DotenvType = Path('') | |
ENV_FILE_SENTINEL: DotenvType = Path('') |
I generally prefer to have module-level constants be all-caps (unless they are type aliases, like the one just above this line, in which case PascalCase is fine). You can ignore this suggestion if preferred, I just think it makes it easier to tell when skimming through code that the variable was a module-level constant since I can't immediately see where it was defined.
Also, can we add documentation of how this sentinel works? I don't understand what the meaning is vs. using None
— I think it should be easy to determine that just from reading source code, and since I can't tell what it's doing by looking at the diff in this PR, I think it would be good to add a comment either in the docstring of DotEnvSettingsSource.__init__
or right under this line defining env_file_sentinel
explaining what the distinction is between providing the value None
and the value env_file_sentinel
.
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.
I've renamed it to ENV_FILE_SENTINEL
and added some comments on top of it.
Please read it and let me know who do you feel.
Note: we have a test_env_file_override_none
for this behavior.
pydantic-settings/tests/test_settings.py
Lines 809 to 820 in 3e93d17
def test_env_file_override_none(tmp_path): | |
p = tmp_path / '.env' | |
p.write_text(test_env_file) | |
class Settings(BaseSettings): | |
a: Optional[str] = None | |
model_config = ConfigDict(env_file=p) | |
s = Settings(_env_file=None) | |
assert s.a is None | |
please update |
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #82 +/- ##
=======================================
Coverage 97.41% 97.41%
=======================================
Files 5 5
Lines 309 309
Branches 67 67
=======================================
Hits 301 301
Misses 6 6
Partials 2 2
☔ View full report in Codecov by Sentry. |
please review |
3cde906
to
626bdc1
Compare
pydantic_settings/sources.py
Outdated
# This is used as default value for `_env_file` in `Settings` class and | ||
# `env_file` in `DotEnvSettingsSource`. Then passing `None` as `_env_file` | ||
# during initialization of `Settings` has a special meaning and it will | ||
# override the `env_file` specified in config. | ||
ENV_FILE_SENTINEL: DotenvType = Path('') |
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.
Can we make this a bit more clear of the exact behavior so I know what will happen without needing to check the logic (right now it says "special meaning", but not what that special meaning is)? Here's an attempt — I haven't checked the logic closely, can you confirm if this is correct?
# This is used as default value for `_env_file` in `Settings` class and | |
# `env_file` in `DotEnvSettingsSource`. Then passing `None` as `_env_file` | |
# during initialization of `Settings` has a special meaning and it will | |
# override the `env_file` specified in config. | |
ENV_FILE_SENTINEL: DotenvType = Path('') | |
# This is used as default value for `_env_file` in `Settings` class and | |
# `env_file` in `DotEnvSettingsSource`. When this value is used, environment | |
# variables will be read from the `model_config`-specified file. Using this | |
# as the default instead of `None` allows us to use `None` to mean that environment | |
# variables should not be read from any env file. | |
ENV_FILE_SENTINEL: DotenvType = Path('') |
Another approach we could take is putting this in the pydantic_settings.main.BaseSettings.__init__
docstring, and then we could shorten/simplify the above comment:
def __init__(
__pydantic_self__,
_case_sensitive: bool | None = None,
_env_prefix: str | None = None,
_env_file: DotenvType | None = env_file_sentinel,
_env_file_encoding: str | None = None,
_env_nested_delimiter: str | None = None,
_secrets_dir: str | Path | None = None,
**values: Any,
) -> None:
"""
TODO: Properly document all additional kwargs here.
When `_env_file == ENV_FILE_SENTINEL`, the `model_config`-specified env file will be used.
When `_env_file is None`, environment variables will not be read from any env file.
"""
I do think we need to document all these keyword arguments eventually, but even if you don't want to do that now, I think it's okay to put a placeholder docstring in like this and it would address my concern.
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.
Oh I just realized that this is documented in the pydantic_settings.main.BaseSettings
docstring. I think we can just augment that docstring as part of this PR if you don't mind. I would change the current value:
_env_file: The env file(s) to load settings values from. Defaults to `Path('')`.
to
_env_file: The env file(s) to load settings values from. Defaults to `Path('')`, which
means that the value from `model_config['env_file']` should be used. You can also pass
`None` to indicate that environment variables should not be loaded from an env file.
or similar
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.
If we change the BaseSettings
docstring, I think we can change the comment here to just
# This is used as default value for `_env_file` in `Settings` class and | |
# `env_file` in `DotEnvSettingsSource`. Then passing `None` as `_env_file` | |
# during initialization of `Settings` has a special meaning and it will | |
# override the `env_file` specified in config. | |
ENV_FILE_SENTINEL: DotenvType = Path('') | |
# This is used as default value for `_env_file` in the `BaseSettings` class and | |
# `env_file` in `DotEnvSettingsSource` so the default can be distinguished from `None`. | |
# See the docstring of `BaseSettings` for more details. | |
ENV_FILE_SENTINEL: DotenvType = Path('') |
This makes source classes inheritable in custom source classes. So, the custom source class doesn't need to override the
__init__
to provide a fallback value from the config.Selected Reviewer: @dmontagu