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

Added better context for can't find config file error message #245

Merged
merged 1 commit into from
Apr 29, 2020

Conversation

snobu
Copy link
Contributor

@snobu snobu commented Mar 17, 2020

No description provided.

@coveralls
Copy link

coveralls commented Mar 17, 2020

Coverage Status

Coverage remained the same at 89.806% when pulling 6d1664f on snobu:patch-1 into 7841022 on theskumar:master.

@bbc2 bbc2 self-assigned this Mar 18, 2020
@snobu
Copy link
Contributor Author

snobu commented Mar 22, 2020

https://github.com/theskumar/python-dotenv/pull/245/files#diff-801425242c4901b4508f6da6283b0100R67

We should probably add Code execution continues with system environment variables since it's not clear if this is recoverable.

Copy link
Owner

@theskumar theskumar left a comment

Choose a reason for hiding this comment

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

@snobu Please update the tests to make the test pass on our Travis-CI. Thanks for your contribution. https://travis-ci.org/github/theskumar/python-dotenv/jobs/663461038#L278-L293

image

@snobu
Copy link
Contributor Author

snobu commented Apr 24, 2020

I fixed all the tests... eventually :)

@snobu
Copy link
Contributor Author

snobu commented Apr 24, 2020

Oh, should we add this other thing or not?
Code execution continues with system environment variables.

@bbc2
Copy link
Collaborator

bbc2 commented Apr 26, 2020

I think this looks good.

I wasn't convinced by this change at first because context is provided by the logging module. When you configure logging in your application, you can choose to show that context. But I suppose that many users don't know that or don't want to do that, so hard-coding that context could be useful.

I wouldn't add the "Code execution continues with system environment variables" message because it makes it look like using environment variables is unusual, which it isn't (as I explain in #164). I would even go as far as turning that logger.warning into logging.info, for the same reason, but I would be happy to merge your change as-is.

snobu added a commit to snobu/python-dotenv that referenced this pull request Apr 26, 2020
snobu added a commit to snobu/python-dotenv that referenced this pull request Apr 26, 2020
@snobu
Copy link
Contributor Author

snobu commented Apr 26, 2020

Sorry this took a while and quite a few build cycles as i thought i'm gonna be a smartass and not clone locally but wing it over the browser... and we all know how that ends.

Should be fine now, i amended the logging level to info, fixed the tests and dropped the square brackets to be consistent with the rest of the verbose output.

'File doesn't exist' doesn't really tell the user much, let's add some context.
@bbc2
Copy link
Collaborator

bbc2 commented Apr 28, 2020

Thanks for the improvements. I squashed the commits together and slightly simplified the test.

There are other things about the output of python-dotenv that can be improved but this is already a good step forward.

@theskumar
Copy link
Owner

I agree with @bbc2 !

@Iain-S
Copy link

Iain-S commented Aug 12, 2021

@bbc2 Would you mind if I changed the logging level back to WARNING? When verbose=True you would expect something to be printed but, with INFO, nothing is printed unless the user has explicitly set the logging level to INFO. This is because the default Python logger level is WARNING. In other words

"""myfile.py"""
import dotenv

dotenv.load_dotenv('/not/a/valid/.env/path', verbose=True)

does nothing right now.

johnbergvall pushed a commit to johnbergvall/python-dotenv that referenced this pull request Aug 13, 2021
'File doesn't exist' doesn't really tell the user much, let's add some context.
@JerPatton
Copy link

@bbc2 Would you mind if I changed the logging level back to WARNING? When verbose=True you would expect something to be printed but, with INFO, nothing is printed unless the user has explicitly set the logging level to INFO. This is because the default Python logger level is WARNING. In other words

"""myfile.py"""
import dotenv

dotenv.load_dotenv('/not/a/valid/.env/path', verbose=True)

does nothing right now.

Hi,
Though it is a bit dated now, I see this is still an open issue.
I quite agree with @Iain-S, would you think that putting back logging.info into logger.warning would make sense ?

I just had a case where my app crashed because the .env file wasn't found and I had no clue it came from this, even after setting the verbose=True option.
It would at least provide some debugging info.

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

6 participants