add opIn for std.process.environment#4941
Conversation
John-Colvin
force-pushed
the
opIn_environment
branch
from
5be747e to
e05eb53
Compare
|
|
John-Colvin
force-pushed
the
opIn_environment
branch
from
e05eb53 to
dfbfac6
Compare
|
ping anyone? Does this need andralex approval for a new symbol? @CyberShadow or @schveiguy perhaps? |
wilzbach
left a comment
wilzbach
left a comment
There was a problem hiding this comment.
I don't know much about the Windows API, so I just stick to point out two nits.
| // GetEnvironmentVariable doesn't specify whether it sets on all | ||
| // failures | ||
| throw new WindowsException(err); | ||
| } |
There was a problem hiding this comment.
There should always be a message for assert(0)s
There was a problem hiding this comment.
Does phobos even support anything other than posix or windows? I only put these in as sanity checks for future porters.
| if (auto var = environment.get("MY_ENV_VAR")) | ||
| doSomething(var); | ||
| ------------- | ||
| */ |
There was a problem hiding this comment.
Any reason why you don't use string?
There was a problem hiding this comment.
Consistency with the rest of the environment API.
There was a problem hiding this comment.
in char[] is preferable over string when immutability is not required (i.e. we don't save a reference to the string, or a slice of it, past the function's return), because this allows calling the function with const or mutable char arrays.
I am not sure - I just tagged it with the motivation of "better be safe than sorry". |
|
I remember I asked for The diff looks good to me though. |
|
The reason I didn't add |
|
ping @andralex |
std/process.d
Outdated
| // New variable | ||
| environment["std_process"] = "foo"; | ||
| assert (environment["std_process"] == "foo"); | ||
| assert ("std_process" in environment); |
There was a problem hiding this comment.
BTW (and I realize you're sticking to existing style) I think Phobos style is to treat assert like a function, not a keyword (i.e. no space before ().
There was a problem hiding this comment.
See #5169 for an enforcement and automatic unification of this assert style ;-)
std/process.d
Outdated
| // New variable | ||
| environment["std_process"] = "foo"; | ||
| assert (environment["std_process"] == "foo"); | ||
| assert ("std_process" in environment); |
|
Changed all the asserts in the file to the correct format |
FWIW #5169 does this for entire Phobos in one go, but it doesn't hurt. |
7 participants
Useful because:
getgeton windows & posixWhy does it not return a
stringwith the variable? Because that would mean it was just an alternative syntax forgetand pretty pointless.Why not do something clever and return a
const(string)*in order to fully mimic an AA? Because I don't think there's any way of doing that even close to efficiently.