-
Notifications
You must be signed in to change notification settings - Fork 89
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
Fix return type of Env.read_env() #111
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be changed to Optional[_BoolType]
?
environs.py
Outdated
@@ -275,6 +275,7 @@ def read_env( | |||
check_path = os.path.join(dirname, env_name) | |||
if os.path.exists(check_path): | |||
return load_dotenv(check_path, stream=stream, verbose=verbose, override=override) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't we still return a DotEnv
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. The return type of dotenv.load_dotenv()
is bool, never aDotEnv
. More specifically it is a literal True
value always.
If we want to keep behavior exactly as it was, then yes, we can return None
and set the return type to Optional[_BoolType]
in this MR, in which case the behavior doesn't change at all compared to what it currently is. This is a very confusing and inconsistent return value though: it means we return None
if a recursive search was done, and no .env was found, and a literal True
in all other cases (including the case when no .env was done and no recursive search was done). We never return a False
. The return value is essentially quite meaningless.
Like said, if the return value is meaningless, it should IMO always be None
. I can and would like to make that change, but it's a breaking change, so it's up to you.
This MR in its current state would change the return value to a literal True
value, which is consistent with dotenv.load_dotenv
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. I was misreading the code for load_dotenv
. Sure enough, it returns True
.
I'm fine returning None
here instead; as you said, the return value should not be used, so I think the code should reflect that.
Can you make that change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, made us return None
always now.
Thanks again! |
I set the return value to True for consistency and backwards compatibility. I recommend though that we set the return value to either always None, or return a boolean that expresses whether the dotenv file was found or not.