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

chore: reverting changes from 217 #261

Closed
wants to merge 1 commit into from
Closed

Conversation

salper
Copy link

@salper salper commented Jun 10, 2018

Be aware that the behavior has been applied to the cli arguments. People who use the cli arguments to disable default values will not be pleased. What do you think about that ?

This fixes #224

@coveralls
Copy link

coveralls commented Jun 10, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling eda6d6d on salper:master into 4f6e274 on mozilla:master.

@A-312
Copy link
Contributor

A-312 commented Dec 7, 2019

Sometime we can't unset a variable environnement then we will set to blank. I think it's better if we keep this usage like default and let people to use their own CustomGetter for env & arg.

According to :

comments on issue

#224 (comment) :

If the environment variable is present and the value of the variable is empty string, the config value should be set to empty string. If the environment variable is missing, the default value should be used. Is that the correct assumption?

#224 (comment) :

Seems right to me.

Me too.

#224 (comment) :

I agree this change warranted a major version. At this point, though, there are probably users relying on both behaviors, so that's water over the damn. What we need to decide is how to move forward.

#224 (comment) :

CustomGetter #313 people rewrite env and arg function like they want.

@salper
Copy link
Author

salper commented Nov 3, 2020

It's been a long time already. Looking at the previously mentioned fork, I'm closing it.

@salper salper closed this Nov 3, 2020
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.

4.0.1 regression
3 participants