-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Amend RFC 517: Add material on std::env #578
Conversation
**Environment variables**: | ||
|
||
* `vars` (renamed from `env`): yields a vector of `(OsStrBuf, OsStrBuf)` pairs. | ||
* `var` (renamed from `getenv`): take a value bounded by `IntoOsStrBuf`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IntoOsStrBuf
or IntoOsStr
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that these APIs will need to go into an OsStrBuf
regardless as they need to be passed to a system API which means they have to be nul-terminated (and a slice won't have the terminator).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These actually all ended up being AsStrBuf
anyway, so I think the RFC just needs to be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make more sense for these APIs to return a Result<String, EnvError>
where EnvError
is:
enum EnvError {
Missing,
NotUnicode(OsString)
}
That way you can quickly and ergonomically get at a String, which is usually what you want, but still get at the OsString
if you really need it (usually to present a good error message).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, to be more precise, I think it makes sense to have two variants of these APIs, one of which provides an Option<OsString>
and one of which provides a Result
.
I would propose var
for the version that returns a Result<String, E> and raw_var
for the one that returns an Option<OsString>
Please let's get the names consistent using Ya, there's a lot of discussion in the previous PR about thread/physical-core count and here. |
I've got a working implementation of There are still a few lingering questions on this RFC, but I'd like to propose marking those APIs as
If I missed something though please let me know! |
|
@brson: |
re: |
An update on this RFC: Use of After discussion above and on IRC, it seems that everyone is basically happy to use Variants for returning `String The idea that @wycats mentioned about However, it focuses just on I suggest that instead we keep |
There seems to be broad enough support for this RFC that I am going to merge it. There are still some components which are going to be left Thanks again for all the feedback everyone, it has all been quite helpful! |
The IO reform RFC is being split into several semi-independent pieces, posted as PRs like this one.
This RFC amendment adds material for
std::env
.Rendered