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

Nested env #3159

Merged
merged 15 commits into from Dec 18, 2021
Merged

Nested env #3159

merged 15 commits into from Dec 18, 2021

Conversation

Air-Mark
Copy link
Contributor

@Air-Mark Air-Mark commented Sep 1, 2021

Change Summary

  • new env_nested_delimiter option in Config class
  • optional _env_nested_delimiter kwarg in init BaseSettings
  • patched EnvSettingsSource
    Depending on env_nested_delimiter trying to find env variables with that delimiter and split those variable into nested dicts. Merge those dicts if they have same nesting structure.
    So lets say we have
class A(BaseSettings):
    foo: str


class B(BaseSettings):
    bar: A


class C(BaseSettings):
    baz: B

now we are able to fill the A.foo with env variable like
BAZ__BAR__FOO='hello'

Related issue number

#2304 #2232

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@MadJlzz
Copy link

MadJlzz commented Sep 22, 2021

Exactly what I was looking for! I believe that pydantic is already doing an excellent job ; removing boiler plate code thanks to this PR will help us centralize settings configuration.

I can help if needed (doc, testing, ...)

cc @daviskirk, because https://github.com/daviskirk/climatecontrol#pydantic seems to have this already implemented.

@Air-Mark
Copy link
Contributor Author

@MadJlzz it is nice to hear that it was useful for you. Sorry for disappearing for almost a moth... I cleaned up code and logic a bit. Added some docs and tests. I think this PR is now ready for submission. And of course It would be great to have some help if needed.

@Air-Mark Air-Mark marked this pull request as ready for review September 25, 2021 14:48
@Air-Mark
Copy link
Contributor Author

please review

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.

Thanks for this, I've wanted this since I first build pydantic, I didn't include it in the first release and never got around to implementing it.

I'm really excited to use this once it's done.

docs/usage/settings.md Show resolved Hide resolved
docs/usage/settings.md Outdated Show resolved Hide resolved
docs/usage/settings.md Outdated Show resolved Hide resolved
docs/usage/settings.md Outdated Show resolved Hide resolved
docs/usage/settings.md Outdated Show resolved Hide resolved
docs/usage/settings.md Outdated Show resolved Hide resolved
docs/usage/settings.md Outdated Show resolved Hide resolved
pydantic/env_settings.py Show resolved Hide resolved
pydantic/env_settings.py Show resolved Hide resolved
pydantic/env_settings.py Outdated Show resolved Hide resolved
@samuelcolvin
Copy link
Member

please update

Air-Mark and others added 2 commits December 11, 2021 22:11
@Air-Mark
Copy link
Contributor Author

please review

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.

A few things to change, you also need to rethink explode_env_vars significantly.

pydantic/env_settings.py Outdated Show resolved Hide resolved
pydantic/env_settings.py Outdated Show resolved Hide resolved
Comment on lines 31 to 38
os.environ.update({
'top': '{"v1": "1", "v2": "2"}',
'v0': '0',
'top__v3': '3',
'top__sub': '{"sub_sub": {"v6": "6"}}',
'top__sub__v4': '4',
'top__sub__v5': '5',
})
Copy link
Member

Choose a reason for hiding this comment

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

as mentioned before, please move this to exec_examples.py

Copy link
Contributor Author

@Air-Mark Air-Mark Dec 12, 2021

Choose a reason for hiding this comment

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

Fixed. Misunderstood you last time about where exactly I should place this.

for idx, key in enumerate(keys):
if idx == len(keys) - 1:
try:
env_val = settings.__config__.json_loads(env_val) # type: ignore
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 why you need this here? Also not sure why we're catching and ignoring TypeError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed about the TypeError we process only env values here, so there are only sting values.
We need this piece because env_val can be a json string. And there could be a situation when you want to mix both approaches of defining nested models:

export TOP__SUB__V5='5'
export TOP='{"sub": {"v6": "6"}}'

So the correct behaviour in my mind was that we should merge those variables into one, instead of overwriting.

Comment on lines 224 to 227
result: Dict[str, Any] = {}
for env_name, env_val in env_vars.items():
keys = env_name.split(self.env_nested_delimiter)
env_var = result
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 think this approach works.

If you change the order of env variables in your test below, the result changes. E.g. put env.set('top__sub__v5', '5') first.

Copy link
Contributor Author

@Air-Mark Air-Mark Dec 12, 2021

Choose a reason for hiding this comment

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

This was the right logic, because this variable should be overwritten by the next one env.set('top', '{"v1": "1", "v2": "2", "sub": {"v5": "xx"}}')
But thank you for pointing on this. Double checked all combinations and yes there was a bug. Redesigned this method and test a bit according to this bug and logic from my answer on your comment above.

@samuelcolvin
Copy link
Member

please update.

@Air-Mark
Copy link
Contributor Author

Thank you for review. Did changes that you been asking for. Improved and redesigned explode_env_vars a bit.
Please review

@samuelcolvin
Copy link
Member

Thanks so much for this, I'm really looking forward to using it.

I've made some changes to the logic to avoid a few errors I found. Please let me know if you have any and feedback or want to make any more changes, otherwie I'll merge over the next few days.

@samuelcolvin samuelcolvin merged commit be24670 into pydantic:master Dec 18, 2021
@samuelcolvin
Copy link
Member

Thanks so much for this, I've actually merged now as I want to include this in v1.9 and I want to get a pre-release version of v1.9 out today.

If you want to make any changes to this, feel free to create new PR.

@Air-Mark
Copy link
Contributor Author

That is awesome. Thank you for review and help. I can use a pre-release version in some projects, and will be glad to help with bugs if needed.

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