Update load_dotenv to return False if no dotenv source is loaded

[br3ndonland]

#164 was closed, but I still find the behavior of load_dotenv() confusing.

The load_dotenv() function is supposed to "Parse a .env file and then load all the variables found as environment variables," according to its docstring. However, the function always returns True, even if no .env file is found or no environment variables are set, because of DotEnv.set_as_environment_variables().

@theskumar commented in #164:

The return value of load_dotenv is undocumented as I was planning to do something useful with it, but could not settle down to one.

I think it make sense to return proper return based on if file was found or not, but if the idea is to just stop execution if no .env is found I think it would make much more sense to just pass it a fail_silently=False and have it raise an exception.

Rather than adding fail_silently=False, which is very similar to verbose=True, I think the easiest way to move forward on this is to update the behavior of verbose=True. This PR will improve load_dotenv(verbose=True) in the following ways:

• Improve DotEnv.set_as_environment_variables() to log a message and return False when no variables are loaded and verbose=True. It will still return True as usual if verbose=False. logger.info() is used for consistency with Added better context for can't find config file error message #245, although I think a warning would be clearer.
• Add test_load_dotenv_empty_file() with tests for verbose=True and verbose=False.
• Update test_load_dotenv_no_file_verbose() to handle the new LookupError exception.

Flake8, Pytest, and Tox (only tried Python 2.7 and 3.7) are currently passing on my branch.

[coveralls]

Coverage increased (+2.6%) to 91.924% when pulling 78c7835 on br3ndonland:load-dotenv-verbose into fa354ce on theskumar:master.

[br3ndonland]

The slight decrease in code coverage is happening because of how I combined the exception and return in main.py L106. The tests actually still pass because of how I set up Pytest context management.

I will separate out the exception handling from the return, maybe adding a raise_error_if_not_found=False argument like find_dotenv(), and will push an amended commit into this PR.

[br3ndonland]

Alright cool, I re-did my commit. It looks like all the checks are passing, and the code coverage actually increased slightly.

[bbc2]

Thank you for trying to improve the situation with regards to missing .env files. I agree it's not great at the moment.

Here's my main concern: For consistency with most other programs, a verbose option shouldn't change what exceptions are raised, only the amount of information displayed.

Changing the return value of load_dotenv is good in my opinion. In this MR, it returns whether any value was loaded, which is probably sensible, but it means that when it returns False, you don't know if the problem comes from a missing file or an empty file (that's the problem of booleans). So perhaps it would be simpler for the user to only return whether a file was found, regardless of whether it has values in it? Or maybe we don't care because there are log messages that show what the problem is?

[br3ndonland]

Thanks for your helpful review.

Here's my main concern: For consistency with most other programs, a verbose option shouldn't change what exceptions are raised, only the amount of information displayed.

That's a great point. We should handle logging and exceptions separately. For example, a user might want to load an empty .env file, and then add values to it with set_key.

when it returns False, you don't know if the problem comes from a missing file or an empty file

Right. We could add separate keyword arguments to load_dotenv to make this clearer:

• raise_error_if_not_found=False: Addresses the missing file issue. load_dotenv could potentially be running find_dotenv, which accepts the raise_error_if_not_found keyword argument.
• raise_error_if_nothing_set=False: Addresses the empty file issue. If the user intends to load variables from a .env, but none are set, a separate exception will be thrown. However, if the user simply wants to load an empty .env file, they can leave this argument False and no exception will be thrown.

The latest commit will add these suggested arguments and avoid throwing exceptions with verbose=True alone. Test coverage will increase also.

I'm happy to keep helping with this until we have a satisfactory API. I'm also happy to work on updating the docs if we do merge these changes.

[br3ndonland]

Hi @bbc2, just wanted to check in about this PR. Is there anything more I can help with?

[bbc2]

Sorry I didn't reply sooner. The issue got kind of stuck in my head because I wasn't really satisfied with the API and didn't really know what else to suggest. This is a difficult issue.

I've thought a bit more now and I think I want to keep the API simple:

• The return value of load_dotenv shouldn't be more complex than "whether an env file was found". Making it depend on whether it's empty (what's an empty env file? 0 bytes? just comments?) or what options are passed is too complex, in my opinion.
• The raise_error_if_not_found option is quite redundant if the return value has the same meaning, so I would rather avoid it, at least for now. I know that find_dotenv already has that option but it also doesn't really make sense there, since find_dotenv will return '' (I would prefer None but that would be a breaking change) if no env file was found, which is easy enough to check (path = find_dotenv(...); assert path).
• The raise_error_if_nothing_set option is too ambiguous. Does it mean that no env file was found? That an env file was found but it was empty? That the env file is not empty but its values are empty or already set in the environment? I think that, although we could choose one, it will necessarily disappoint some people, so I would rather also avoid that option. Users who want that much visibility on their env file(s) should use dotenv_values instead, and then override os.environ themselves (or not).

What do you think?

I think I would be happy to review code that just fixes the return value of load_dotenv to mean "was an env file found?" instead of always returning True. For anything else, I would like to discuss it.

[br3ndonland]

Thanks for your feedback @bbc2. I agree that this is challenging, and that keeping the API simple will help us move forward. The latest commits will remove some of the complications introduced in 1011590 and b5a9613:

• Remove raise_error_if_not_found and raise_error_if_nothing_set from load_dotenv
• Remove the exception from load_dotenv
• Restore previous return behavior of DotEnv.set_as_environment_variables (always returns True, even if dict is empty)

The simplest solution I have come up with so far is to pass dotenv_path to find_dotenv() first.

# old (docstring omitted for simplicity)
def load_dotenv(dotenv_path=None, stream=None, verbose=False, override=False, interpolate=True, **kwargs):
    f = dotenv_path or stream or find_dotenv()
    return DotEnv(f, verbose=verbose, interpolate=interpolate, **kwargs).set_as_environment_variables(override=override)

# new
def load_dotenv(dotenv_path=None, stream=None, verbose=False, override=False, interpolate=True, **kwargs):
    f = find_dotenv(filename=str(dotenv_path)) or stream or find_dotenv()
    env = DotEnv(f, verbose=verbose, interpolate=interpolate, **kwargs).set_as_environment_variables(override=override)
    return env if f else False

If the user runs load_dotenv(dotenv_path="filename"), resulting in f = dotenv_path, the path will now be passed to find_dotenv(). If no files or streams are available, load_dotenv will return False.

[bbc2]

Thank you for the update. Here are some comments.

[br3ndonland]

The maintainer was unwilling to implement the breaking change proposed by this PR.