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

Use config values as default in PydanticBaseEnvSettingsSource.__init__ #100

Merged
merged 2 commits into from
Jul 5, 2023

Conversation

hramezani
Copy link
Member

@hramezani hramezani commented Jul 4, 2023

Fixes #98

Selected Reviewer: @samuelcolvin

@hramezani
Copy link
Member Author

please review

Copy link
Contributor

@Viicos Viicos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I don't think this addresses the issue unfortunately. Here's a MRE of what should be expected:

class CustomEnvSettingsSource(EnvSettingsSource):
    pass

class Settings(BaseSettings):
    foo: str

    @classmethod
    def settings_customise_sources(
        cls,
        settings_cls: type[BaseSettings],
        init_settings: PydanticBaseSettingsSource,
        env_settings: PydanticBaseSettingsSource,
        dotenv_settings: PydanticBaseSettingsSource,
        file_secret_settings: PydanticBaseSettingsSource
    ) -> tuple[PydanticBaseSettingsSource, ...]:
        custom_env_settings = CustomEnvSettingsSource(settings_cls)  # CustomEnvSettingsSource should be able to use config values either from `model_config` or values given at instantiation
        return init_settings, custom_env_settings, dotenv_settings, file_secret_settings

os.environ["prefix_foo"] = "test_foo"

s = Settings(_env_prefix="prefix_")
assert s.foo == "test_foo"

What the added tests show is that CustomEnvSettingsSource is able to read values from model_config, but not from instantation arguments 😕

tests/test_settings.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (107e8de) 97.42% compared to head (8b6daf5) 97.42%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #100   +/-   ##
=======================================
  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/sources.py 97.70% <100.00%> (ø)

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

@hramezani
Copy link
Member Author

Thanks!

I don't think this addresses the issue unfortunately. Here's a MRE of what should be expected:

class CustomEnvSettingsSource(EnvSettingsSource):
    pass

class Settings(BaseSettings):
    foo: str

    @classmethod
    def settings_customise_sources(
        cls,
        settings_cls: type[BaseSettings],
        init_settings: PydanticBaseSettingsSource,
        env_settings: PydanticBaseSettingsSource,
        dotenv_settings: PydanticBaseSettingsSource,
        file_secret_settings: PydanticBaseSettingsSource
    ) -> tuple[PydanticBaseSettingsSource, ...]:
        custom_env_settings = CustomEnvSettingsSource(settings_cls)  # CustomEnvSettingsSource should be able to use config values either from `model_config` or values given at instantiation
        return init_settings, custom_env_settings, dotenv_settings, file_secret_settings

os.environ["prefix_foo"] = "test_foo"

s = Settings(_env_prefix="prefix_")
assert s.foo == "test_foo"

What the added tests show is that CustomEnvSettingsSource is able to read values from model_config, but not from instantation arguments 😕

You can pass env_prefix in custom source initialization.

custom_env_settings = CustomEnvSettingsSource(settings_cls, env_prefix="prefix_")

or define it in config as I shared an example in the issue

@Viicos
Copy link
Contributor

Viicos commented Jul 4, 2023

You can pass env_prefix in custom source initialization.

custom_env_settings = CustomEnvSettingsSource(settings_cls, env_prefix="prefix_")

or define it in config as I shared an example in the #98 (comment)

But the env_prefix is then hardcoded into the code, and not configurable from instantiation arguments

I understand the MRE I gave here can't be supported, as trying to support it might lead to too many breaking changes.
I can still offer a PR and see if it's worth it 😉

@hramezani
Copy link
Member Author

@Viicos _env_prefix and the other instantiation arguments are for settings class init not the source class init.

If you define a custom source, you need to initialize it in settings_customise_sources and then you can pass args there.

I understand the MRE I gave #100 (review) can't be supported, as trying to support it might lead to too many breaking changes.
I can still offer a PR and see if it's worth it 😉

I would be happy to take a look at that. we can merge it if it is an easy one. because as I mentioned above, you can initialize a custom source in settings_customise_sources and it is not block you.

@hramezani hramezani requested review from Kludex and dmontagu July 4, 2023 09:33
@Kludex Kludex merged commit 3f613ae into main Jul 5, 2023
20 checks passed
@Kludex Kludex deleted the custom_source_config branch July 5, 2023 06:16
@Kludex
Copy link
Member

Kludex commented Jul 5, 2023

I merged without seeing the messages above, sorry. Hope is cool.

@Viicos
Copy link
Contributor

Viicos commented Jul 5, 2023

I merged without seeing the messages above, sorry. Hope is cool.

Yes this is still an improvement, thanks for merging

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.

Usage of SettingsConfigDict values in custom sources
5 participants