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

Bugfix: Doesn't load env file through recursion #157

Closed
wants to merge 6 commits into from
Closed

Bugfix: Doesn't load env file through recursion #157

wants to merge 6 commits into from

Conversation

timoklimmer
Copy link

Another commit to fix a bug that prevents loading of custom-named .env files in upper directories. Follow-up to commit #155

@timoklimmer
Copy link
Author

Any updates?

@sloria
Copy link
Owner

sloria commented May 25, 2020

Sorry I haven't been able to review this in-depth. Haven't been able to find the focused time that this requires. This is more complexity than I was anticipating for this issue.

Without having looked into this thoroughly, I'm wondering if we can use find_dotenv to do most of the searching logic? @timoklimmer Can you investigate this?

@sloria
Copy link
Owner

sloria commented May 25, 2020

Also, for future reference: no need to send a new PR every time you make changes/respond to code review. Instead make a new branch for your PR and commit on that. I can take care of the squash and merge when all is said and done.

@timoklimmer
Copy link
Author

"when all is said and done" - honestly, I don't want to have an endless conversation here. The only thing I am asking for is to get a small but relevant bug fixed in your package so I can use it without writing workaround code in my code. Others and I have already invested quite some time by writing PRs incl. the back-and-forth communication we had plus the unit tests, and now my fair expectation is that we get out the fix soon.

I understand you don't have much time, and so do I. So my ask to you would be not to ask for new commits over and over about things you can quickly do on your own. Rather take the little time you have and adjust the PR as you think is needed so we can out the fix asap. Looking at the other PRs, there seem to be other people waiting for a fix too. They even provided PRs...

Your idea to use find_dotenv does not work because it cannot cope with relative directories. A path like "tests/.custom.env" as in your unit tests leads to an exception. That's why I introduced merge_absolute_and_relative_path().

@sloria
Copy link
Owner

sloria commented May 26, 2020

I do appreciate all the time you've put into this. It is my goal, too, to land the fix for this bug. It's not my intention to set up artificial barriers by nitpicking, etc.

I often do make small fixups myself if those are the only things to address before merging. That's not the case for this PR. There are multiple approaches to consider, and looking into these approaches and their side effects takes focused time. The approach in this PR adds a fair amount of custom code which--on a cursory glance--seems unnecessary given the functions available in pathlib and dotenv. It may be the case that this PR is the best approach, but I want to make sure that alternatives are explored. I actually was planning to do this exploration myself this past weekend but wasn't able to find the time due to personal obligations.

For now, I'd recommend working around the issue by providing a direct path to the target .env file.

from pathlib import path

from environs import Env

HERE = Path(__file__).parent 
ENV = HERE / "path" / "to" / "custom.env"

env = Env()
env.read_env(ENV)

Hopefully I'll have some time this week to look further into this. All I ask from you is patience. We're on the same team here.

sloria added a commit that referenced this pull request May 28, 2020
@sloria sloria closed this in #159 May 28, 2020
sloria added a commit that referenced this pull request May 28, 2020
* Add failing tests from #157

* Clean up tests and add additional cases

* Implement the fix

* Use pathlib
@sloria
Copy link
Owner

sloria commented May 28, 2020

#159 has the bug fix and is released in 8.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants