#5186 env var expansion #5434
#5186 env var expansion #5434
Conversation
There was a problem hiding this comment.
Looks very good in general, but there are some style issues to be fixed and some design decisions to be made. Most important design decisions are these:
- Should variable be expanded already before splitting lines or handling comments? I believe yes, because it would have small benefits and it would fix the current bug with
-N$value. - Should we support
#expandvars:as well or only# expandvars:? I don't care too much, but I believe making the space optional would be good. - Should we support
${var=default}style? I think it would be nice, but it can also be added later if there are concrete needs. - Should we support
%var%style on Windows? I don't consider it that important.
| elif line and not line.startswith("#"): | ||
| args.append(line) | ||
| args.append(self._expand_vars(line) if expand_vars else line) |
There was a problem hiding this comment.
It would be simpler to expand variables from the whole file before even splitting it into lines than doing that for each line. It would make it impossible to expand variables only from some parts of the lines, but I actually don't think that's a good idea in general. I'll comment that separately later.
| if not first_line.startswith('# expandvars:'): | ||
| return False | ||
|
|
||
| pragma_value = first_line.split(':', 1)[1].strip().lower() |
There was a problem hiding this comment.
Should we also support #expandvars:? What about # expandvars: where the space is actually (and most likely accidentally) a non-break space? An easy way to support them would be using regular expressions. They are often complicated, but in this case I consider the resulting code simpler:
match = re.fullmatch('#\s*expandvars:\s*(.*?)\s*', lines[0], flags=re.IGNORECASE)
if not match:
return False
value = match.group(1).lower()
return value not in ("false", "no", "off", "")There was a problem hiding this comment.
i think that would be : .upper() if we are goind to use FALSE_STRINGS instead of ("false", "no", "off", "")
otherwise , resolved
| separator = self._get_option_separator(line) | ||
| if not separator: | ||
| return [line] | ||
| option, value = line.split(separator, 1) | ||
| if separator == " ": | ||
| value = value.strip() | ||
| if expand_vars: | ||
| value = self._expand_vars(value) |
There was a problem hiding this comment.
This implementation expands variable only from option values, not from option names. I don't think that's a good idea for the following reasons:
- On the commend line you can use
--$name valuejust fine so the implementation is inconsistent with that. I see no problems with supporting variables in option names either. - The implementation doesn't work correctly when using short options without a separator like
-N$value. Trying to fix this would be pretty complicated, because short options can be combined like-XN$value. Related to this, at least the-N$valueusage should get a test! - If variables are parsed everywhere, it can be done before splitting content to lines which simplifies the overall implementation.
- If variables are parsed as the first step, it allows conditional usage like
where
$COND --name Example$CONDcould be set to#to exclude the line. Not sure is this that useful, but could come handy in some cases.
| def _expand_vars(self, value): | ||
| # Handle escaping with Template.safe_substitute | ||
| # (fail gracefully without raising errors for missing variable | ||
| return Template(value).safe_substitute(os.environ) |
There was a problem hiding this comment.
- Using
Template.safe_substituteinstead ofos.path.expandvarsis probably a good idea, but the latter would support%name%substitution on Windows. If that's important, we ought to be able to to extendTemplateto support it. - One benefit of using
Templateis that we could extend it to support${name=default}style somewhat easily. It could even be part of this enhancement, but it can also be added later. - No need to explain how
safe_substitureworks here.
There was a problem hiding this comment.
thanks for the review
i see the trade-oof between using Template.safe_substitute and os.path.expandvars
1- i thought a bit about extending Template to support both ${var} and %var% formats, the solution i saw is :
creating like " EnvVarTemplate subclass " that extend functionality through inheritance to support %var% formats. I think that this would require regex pattern. however, I'm still wondering if adding windows style variable support critical for the current enhancement? I think that i can push this enhancement later since it will requires more tests
2- i' am considering adding this support ( ${name=default} ) but i don't see another solution other then this :
def _expand_vars(self, value):
result = Template(value).safe_substitute(os.environ)
regexpatter, = r'\${([^}]+?)=([^}]*)}'
def replace_with_default_value(match):
var, default = match.groups()
return os.environ.get(var, default)
return re.sub(regexpatter, replace_with_default_value, result)
3- intended to make a comment for design reminder
|
I mentioned in my review that we could add support for defaults with syntax like from collections.abc import Mapping
from string import Template
class TemplateWithDefaults(Template):
braceidpattern = r'(?a:[_a-z][_a-z0-9]*(=[^}]*)?)'
class NamespaceWithDefaults(Mapping):
def __init__(self, data=None, **extra):
self.data = {**(data or {}), **extra}
def __getitem__(self, key):
if '=' in key:
key, default = key.split('=', 1)
return self.data.get(key, default)
return self.data[key]
def __iter__(self):
return iter(self.data)
def __len__(self):
return len(self.data)
# Test the above:
tmpl = TemplateWithDefaults('${x}, ${y=bär} and ${z}')
ns = NamespaceWithDefaults(x='foo')
print(tmpl.safe_substitute(ns)) # => 'foo, bär and ${z}' |
#5186