Environment Variable Expansion #576
Conversation
|
Woops this broke things on Windows, it seems. I'll ping when fixed. |
|
Thanks @adqm. Did a quick review and it seemed good. Didn't see the win error. Just ping. |
|
Okay, @scopatz, ready for another look, I think. Changes from last time:
|
| i, j = m.span(0) | ||
| name = m.group(1) | ||
| if name.startswith(start) and name.endswith(end): | ||
| name = eval(name[1:-1]) # change for non-POSIX ${...} |
There was a problem hiding this comment.
ast.literal_eval() maybe? Or is this a full eval? Also do you need the current locals and globals? Seems like an empty execution context is desirable here.
There was a problem hiding this comment.
I think this has to be a full eval, but I think it should happen in the current locals and globals.
My thought is that if we have x = "HOME", both of the following should work:
>>> ${x}
>>> echo "${x}"
literal eval wouldn't handle that case.
There was a problem hiding this comment.
Understood. Got it. That will be super cool to have.
There was a problem hiding this comment.
But I think you then want to use the xonsh eval or at least the __xonsh_ctx__ as the execution context. As written this will use the function's globals and locals.
There was a problem hiding this comment.
Yes, sorry; that was what I meant by the comment above; we will want to eval in the xonsh context.
add slashes to quoted strings with, e.g., ~ in them
| return path | ||
| if not _varprog: | ||
| import re | ||
| _varprog = re.compile(r'\$(\w+|\{[^}]*\})', re.ASCII) |
There was a problem hiding this comment.
This doesn't handle %name% completion on windows, which someone will probably complain about :) https://docs.python.org/3.5/library/os.path.html#os.path.expandvars Shouldn't be too hard to modify.
There was a problem hiding this comment.
Ah, yes. That's what I get for copying from posixpath.py and not even looking at the windows equivalent. I can work that in.
Nope that was an oversight! Thanks for fixing. |
|
Thanks for all of the hard work here! |
|
Alright, ready for another look. This time I:
|
| if self._orig_env is not None: | ||
| os.environ.clear() | ||
| os.environ.update(self._orig_env) | ||
| self._orig_env = None |
There was a problem hiding this comment.
I think that these are still potentially useful to users. Do you mind putting them back in?
|
Looks great to me, other than the method removal. Basically ready to merge. |
|
I can put them back in if you'd like too. |
|
I just put them back in, so we should be good to go, I think (assuming the tests pass). Let me know if you have any other thoughts/suggestions. I'll also take a look at your history PR tonight. |
Environment Variable Expansion
|
Awesome! |
This changeset:
I think it is all working, and I hope I didn't break anything else.
Also, separately, I'm not quite sure how the Travis and Appveyor autobuild things work, but I am trying here to make them run in quiet mode to avoid printing a dot for each of the test cases (there are really a lot of them now, after the last PR was merged, and printing the dots really seems to slow things down).