tiltfile: add os.getenv, os.putenv, os.unsetenv. Fixes https://github.com/windmilleng/tilt/issues/2564#3329
Conversation
fcf4b19 to
644a0bc
Compare
|
actually, this isn't quite right, one sec |
644a0bc to
6786b1a
Compare
|
ok, fixed. I am...not happy about how much code I needed to duplicate from starlark to get this to work right. The core problem is that we need to make sure updates to the dict are reflected in the environment. The code in environ_methods is basically just copied-and-pasted mechanically, where I just replaced *Dict with Environ (since they use the same API) |
|
(There's obviously a simpler implementation possible where *Dict supports different backing stores, but I'm not sure starlark would accept that upstream) |
6786b1a to
710af3b
Compare
| var defaultVal starlark.Value = starlark.None | ||
| err := starkit.UnpackArgs(t, fn.Name(), args, kwargs, | ||
| "varname", &varname, | ||
| "value?", &defaultVal, |
There was a problem hiding this comment.
| "value?", &defaultVal, | |
| "default?", &defaultVal, |
There was a problem hiding this comment.
ahhhh...good catch! this doc
https://docs.python.org/2/library/os.html
says it's value but this doc says it's default
https://docs.python.org/3/library/os.html
and when i tried it out in a real python interpreter, they all only accepted 'default' :rolledeyes:
|
|
||
| varnameStr, ok := value.AsString(varname) | ||
| if !ok { | ||
| return nil, fmt.Errorf("varname must be a string, actual: %s", varname) |
There was a problem hiding this comment.
Does defaultVal also need to be enforced to be a string?
There was a problem hiding this comment.
surprisingly no!
python -c "import os; print(os.getenv('x', {}))"
{}
710af3b to
efe1661
Compare
2 participants
Hello @jazzdan,
Please review the following commits I made in branch nicks/ch6958/putenv:
fcf4b19 (2020-05-12 17:01:51 -0400)
tiltfile: add os.getenv, os.putenv, os.unsetenv. Fixes #2564
Code review reminders, by giving a LGTM you attest that: