Use default values in POSIX-style variable expansions#143
Use default values in POSIX-style variable expansions#143gergelypolonkai wants to merge 1 commit intotheskumar:masterfrom
Conversation
gergelypolonkai
force-pushed
the
master
branch
from
c901a1e to
a0b2bd8
Compare
gergelypolonkai
force-pushed
the
master
branch
from
a0b2bd8 to
cc8689d
Compare
|
|
||
| # 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.
It is; it comes from the for loop that iterates over values right after the definition of _re_sub_callback(). Also see the end of the comment above this line.
That’s exactly the reason i wrote it feels hacky.
There was a problem hiding this comment.
Oh, is it in the scope of the parent function? 🤢
| # or ${K:-default} as the return value. Note that k comes from the for loop below | ||
| if name == k: | ||
| # If we have a default set, return it | ||
| if default: |
There was a problem hiding this comment.
That was the original line of code; i don’t know why i changed it, but i will put that back in place.
|
|
||
| def resolve_nested_variables(values): | ||
| def _replacement(name): | ||
| def _replacement(name, default=None): |
There was a problem hiding this comment.
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.
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.
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.
|
It seems the tests fail on 3.3 regardless of my change. I wonder why it says anything about IPython… |
|
|
||
| # 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.
It's already tried to get name from os.environ. Why try again here?
theskumar
force-pushed
the
master
branch
2 times, most recently
from
f628767 to
f9863d3
Compare
|
Sorry I never really looked into this PR. It is now superseded by #248. |
4 participants
An attempt to continue #30 with default values
It’s not perfect, but it works as i expect it to work. For example, using
kin_replacement()feels a bit hacky.What do you think?
If you like the basic idea, i can add tests, documentation, etc. to this PR.