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: add support multiple dotenv files (#1497) #3222

Merged
merged 18 commits into from Aug 12, 2022

Conversation

rekyungmin
Copy link
Contributor

@rekyungmin rekyungmin commented Sep 16, 2021

Change Summary

Allow BaseSettings to accept multiple .env files in Config as a list or tuple.
The first value takes priority over the second value., this i changed below, the last file now takes priority.

Example:

.env file:

debug_mode=true
host=localhost
port=8000

.env.prod file:

debug_mode=false
host=https://example.com/services

test code:

from pydantic import BaseSettings


class Settings(BaseSettings):
    debug_mode: bool
    host: str
    port: int

    class Config:
        # `.env.prod` takes priority over `.env`
        env_file = ['.env', '.env.prod']  # (undated to reflect newer behaviour)
        env_file_encoding = 'utf-8'

s = Settings()
print(s.dict())
# {'debug_mode': False, 'host': 'https://example.com/services', 'port': 8000}

Related issue number

#1497

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

Allow BaseSettings to accept multiple dotenv files in Config as a list
or tuple.
@rekyungmin rekyungmin changed the title feat: support multiple dotenv files feat: add support multiple dotenv files (#1497) Sep 16, 2021
@rekyungmin
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.

otherwise LGTM.

docs/usage/settings.md Show resolved Hide resolved
pydantic/env_settings.py Outdated Show resolved Hide resolved
pydantic/env_settings.py Outdated Show resolved Hide resolved
pydantic/env_settings.py Outdated Show resolved Hide resolved
pydantic/env_settings.py Outdated Show resolved Hide resolved
@@ -134,30 +136,47 @@ def __repr__(self) -> str:
class EnvSettingsSource:
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would be smarter to split dotenv processing into a separate source?

I suggested the same thing here but this case is more clear cut.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate on it?

Copy link
Member

Choose a reason for hiding this comment

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

See how sources work in settings. I propose moving dotenv logic into a whole new class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, but it will break backwards compatibility.
This is because customise_sources needs to be changed to take 4 callables as arguments.

Copy link
Member

Choose a reason for hiding this comment

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

let's leave to v2.

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

Please update.

@samuelcolvin samuelcolvin mentioned this pull request Dec 12, 2021
5 tasks
@rekyungmin
Copy link
Contributor Author

please review

@rekyungmin
Copy link
Contributor Author

rekyungmin commented Dec 26, 2021

Thanks for the review. Please review.

@phillipuniverse
Copy link

@rekyungmin @samuelcolvin IMO this is counterintuitive, I would expect the files to be loaded in the order in which they are defined which would mean the last one in the list gets the final say. So instead of this:

for f in reverse(env_file):
    props = load_dotenv(f)

I would expect this:

for f in env_file:
    props = load_dotenv(f)

This also matches the semi-documented behavior for multiple sources in python-dotenv.

As long as it's consistent I suppose not a huge deal either way, I'm just excited to see this feature!! What's left here to merge?

@rekyungmin
Copy link
Contributor Author

@rekyungmin @samuelcolvin IMO this is counterintuitive, I would expect the files to be loaded in the order in which they are defined which would mean the last one in the list gets the final say. So instead of this:

for f in reverse(env_file):
    props = load_dotenv(f)

I would expect this:

for f in env_file:
    props = load_dotenv(f)

This also matches the semi-documented behavior for multiple sources in python-dotenv.

As long as it's consistent I suppose not a huge deal either way, I'm just excited to see this feature!! What's left here to merge?

In Python, it is common for the rightmost value to win (e.g. unpacking, dict union).
However, in customize_sources, the leftmost value wins.
I think the left win strategy is better for consistency in the settings module.

@samuelcolvin, what do you think?

@lucafaggianelli
Copy link

Hey, any news on this PR? would love to see it merged! :)

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.

Very nearly there, one tiny comment, but more generally I agree with @phillipuniverse - the right most entry in the list/tuple should "win".

Otherwise this is ready to go.

Please update.

pydantic/main.py Outdated
@@ -337,7 +337,7 @@ def __init__(__pydantic_self__, **data: Any) -> None:
object_setattr(__pydantic_self__, '__fields_set__', fields_set)
__pydantic_self__._init_private_attributes()

@no_type_check
@no_type_check # noqa: C901 (ignore complexity)
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
@no_type_check # noqa: C901 (ignore complexity)
@no_type_check

@samuelcolvin
Copy link
Member

please update.

@samuelcolvin samuelcolvin enabled auto-merge (squash) August 12, 2022 12:03
@samuelcolvin
Copy link
Member

Thanks @rekyungmin for this, and everyone for your feedback.

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

5 participants