-
Notifications
You must be signed in to change notification settings - Fork 490
Use default values in POSIX-style variable expansions #143
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -181,21 +181,42 @@ def unset_key(dotenv_path, key_to_unset, quote_mode="always"): | |
|
|
||
|
|
||
| def resolve_nested_variables(values): | ||
| def _replacement(name): | ||
| def _replacement(name, default=None): | ||
| """ | ||
| get appropriate value for a variable name. | ||
| first search in environ, if not found, | ||
| then look into the dotenv variables | ||
| """ | ||
| ret = os.getenv(name, new_values.get(name, "")) | ||
| return ret | ||
|
|
||
| # If name is set as an environment variable, use its value | ||
| if name in os.environ: | ||
| return os.getenv(name) | ||
|
|
||
| # If name is the same as k, we need to handle the situation special, or we will have ${K} | ||
| # or ${K:-default} as the return value. Note that k comes from the for loop below | ||
| if name == k: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is; it comes from the for loop that iterates over That’s exactly the reason i wrote it feels hacky. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, is it in the scope of the parent function? 🤢 |
||
| # If we have a default set, return it | ||
| if default: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was the original line of code; i don’t know why i changed it, but i will put that back in place. |
||
| return default | ||
|
|
||
| # Otherwise, return an empty string | ||
| return '' | ||
|
|
||
| # If all the above failed, try getting name from the environments, revert to the value | ||
| # from values, and if it’s not in values yet, revert to the default value | ||
| return os.getenv(name, values.get(name, default or '')) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's already tried to get name from os.environ. Why try again here? |
||
|
|
||
| def _re_sub_callback(match_object): | ||
| """ | ||
| From a match object gets the variable name and returns | ||
| the correct replacement | ||
| """ | ||
| return _replacement(match_object.group()[2:-1]) | ||
|
|
||
| replacement = match_object.group()[2:-1].split(':-', 1) | ||
| name = replacement.pop(0) | ||
| default = replacement[0] if replacement else None | ||
|
|
||
| return _replacement(name, default=default) | ||
|
|
||
| new_values = {} | ||
|
|
||
|
|
||
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.
How about some unit tests?
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.
If you like the general idea, i can go on and write the tests and expand the documentation with examples and caveats.
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.
If you like the general idea, i can go on and write tests and expand the documentation with the new functionality and some caveats.
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.
tests would help to clarify what the general idea is.
Note that I'm not a contributor to this library, so my opinion doesn't carry any weight.