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

feat: adding json, yaml and toml sources #211

Merged
merged 18 commits into from
Feb 16, 2024

Conversation

Smixi
Copy link
Contributor

@Smixi Smixi commented Jan 20, 2024

feat: #210

Selected Reviewer: @samuelcolvin

Copy link

codecov bot commented Jan 20, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (cc6dc25) 97.61% compared to head (90a3b4a) 97.63%.
Report is 3 commits behind head on main.

Files Patch % Lines
pydantic_settings/sources.py 97.01% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #211      +/-   ##
==========================================
+ Coverage   97.61%   97.63%   +0.01%     
==========================================
  Files           5        5              
  Lines         336      423      +87     
  Branches       71       89      +18     
==========================================
+ Hits          328      413      +85     
- Misses          6        7       +1     
- Partials        2        3       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Smixi Smixi marked this pull request as ready for review January 21, 2024 01:24
@samuelcolvin
Copy link
Member

I think you mean toml, not JSON - please update the title.

@Smixi Smixi changed the title feat: adding json and yaml sources feat: adding json and yaml and toml sources Jan 21, 2024
@Smixi Smixi changed the title feat: adding json and yaml and toml sources feat: adding json, yaml and toml sources Jan 21, 2024
@Smixi
Copy link
Contributor Author

Smixi commented Jan 21, 2024

in fact it have the three of them :D.

please review

@hramezani
Copy link
Member

hramezani commented Jan 22, 2024

Thanks @Smixi for the PR 🙏

A couple of things to address:

  • If the dependency is installed, we need to add the source class by default in settings_customise_sources. So, no need to override the settings_customise_sources to use the new source classes. They should have less priority than the existing sources to be backward-compatible.
  • CI is broken for Python 3.12. it seems the pyyaml 6.0 can't be installed. Have you tried the pyyaml 6.0.1?
  • Documentation has to be added in https://github.com/pydantic/pydantic-settings/blob/main/docs/index.md
  • You've added the JsonConfigSettingsSource as well. not sure we want that. @samuelcolvin what do you think?

@@ -19,10 +20,48 @@
from pydantic_settings.utils import path_type_label

if TYPE_CHECKING:
if sys.version_info.minor >= 11:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if sys.version_info.minor >= 11:
if sys.version_info >= (3, 11):

@Smixi
Copy link
Contributor Author

Smixi commented Jan 22, 2024

Thanks @Smixi for the PR 🙏

A couple of things to address:

  • If the dependency is installed, we need to add the source class by default in settings_customise_sources. So, no need to override the settings_customise_sources to use the new source classes. They should have less priority than the existing sources to be backward-compatible.
  • CI is broken for Python 3.12. it seems the pyyaml 6.0 can't be installed. Have you tried the pyyaml 6.0.1?
  • Documentation has to be added in https://github.com/pydantic/pydantic-settings/blob/main/docs/index.md
  • You've added the JsonConfigSettingsSource as well. not sure we want that. @samuelcolvin what do you think?

If they are default loaded, it means the default settings class is in settings_customise_sources, which will result in the API breaking (people having custom class order will fail). If I don't include it in the settings_customise_sourcesthen it means that the order is not configurable by the user without instantiating yourself the TomlSourceSettings in your order.

@hramezani
Copy link
Member

Here is the source settings priority right now:

  • init_settings
  • env_settings
  • dotenv_settings
  • file_secret_settings

So, if we add new sources at the end there shouldn't be a breaking change and there will be new sources that users can use by defining the file path and encoding in config.

It means that releasing the new pydantic-settings won't break existing settings classes and users can benefit from new sources by defining the required configs.

@Smixi
Copy link
Contributor Author

Smixi commented Jan 22, 2024

My concerns is more about user having their own custom orders. They will need to reimplement the settings_customise_sources to add the new parameters (like I did in the tests) ?
Pyaml 6.0.1 seems to fix the issue on 3.12.
Still the doc to do.

@hramezani
Copy link
Member

My concerns is more about user having their own custom orders. They will need to reimplement the settings_customise_sources to add the new parameters (like I did in the tests) ?

That's a good point. didn't think about this.
So, please revert the change and document how to use the new sources. sorry for this

@Smixi
Copy link
Contributor Author

Smixi commented Jan 23, 2024

@hramezani That should be good. I just don't know how to deal with the coverage.

return {}
if isinstance(files, (str, os.PathLike)):
files = [files]
vars: dict[str, Any | None] = {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
vars: dict[str, Any | None] = {}
vars: dict[str, Any] = {}

return vars

@abstractmethod
def _read_file(self, path: Path) -> dict[str, Any | None]:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def _read_file(self, path: Path) -> dict[str, Any | None]:
def _read_file(self, path: Path) -> dict[str, Any]:

@@ -664,6 +704,103 @@ def __repr__(self) -> str:
)


class ConfigFileSourceMixin(ABC):
def _read_files(self, files: PathType | None) -> dict[str, Any | None]:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def _read_files(self, files: PathType | None) -> dict[str, Any | None]:
def _read_files(self, files: PathType | None) -> dict[str, Any]:

Copy link
Contributor Author

@Smixi Smixi Jan 31, 2024

Choose a reason for hiding this comment

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

As in env settings, I think this is allowed in config files to have null values, as it's defined in json and in yaml specs ?
Or is this because None is included in any ?

Changed


def _read_file(self, file_path: Path) -> dict[str, Any | None]:
with open(file_path, encoding=self.yaml_file_encoding) as yaml_file:
import_yaml()
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it better to move import_yaml out of the block to check the import before opening the file?

@hramezani
Copy link
Member

@hramezani That should be good. I just don't know how to deal with the coverage.

I think it can be tested like the old dotenv test in Pydantic V1

and then you can add a new step to the CI to uninstall deps and run the tests to run the above tests

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@hramezani
Copy link
Member

LGTM. Thanks @Smixi.

@samuelcolvin I will keep this PR open. please take a look if you want.

@korolkevich
Copy link

Hi @Smixi,
Thank you for your work.

I have a question:
If we have nested YAML configuration:
app: pass: pass login: login
How do we replace the default configuration with environment variables, as it is done in Dynaconf (app__pass='new_val' or app.pass='new_val'), and handle deeper configurations? I couldn't find this in your PR.

@Smixi
Copy link
Contributor Author

Smixi commented Feb 11, 2024

@korolkevich This PR is only a loader for variables.
I think your use case is more about using this mecanism and declare env_settings before the yaml setting source

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise LGTM.

Thank you so much.

docs/index.md Outdated
foobar = "Hello"
[nested]
nested_field = "world!"
"""
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what this is?

@hramezani hramezani merged commit 8b92f61 into pydantic:main Feb 16, 2024
19 of 20 checks passed
@hramezani
Copy link
Member

Thanks @Smixi

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