Unify the docs of std::env::{args_os, args} more #84551
Conversation
|
r? @yaahc (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
library/std/src/env.rs
Outdated
| /// does on macOS and Windows. | ||
| /// | ||
| /// Note that the returned iterator will not panic during iteration if any argument to the | ||
| /// process is not valid Unicode. For more safety, |
There was a problem hiding this comment.
"For more safety" is a bit confusing here, since this fn isn't unsafe. Perhaps "To get arguments as UTF-8"?
There was a problem hiding this comment.
I changed it similarly to that.
|
Looks good to me, thank you for the clarification ^_^ @bors r+ |
|
📌 Commit 82b6983 has been approved by |
bors
added
S-waiting-on-bors
Rollup of 4 pull requests Successful merges: - rust-lang#84120 (Stabilize Duration::MAX) - rust-lang#84523 (Stabilize ordering_helpers.) - rust-lang#84551 (Unify the docs of std::env::{args_os, args} more) - rust-lang#84574 (rustdoc: Fix typos in maybe_inline_local fn) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
library/std/src/env.rs
Outdated
| /// process is not valid Unicode. For more safety, | ||
| /// Note that the returned iterator will not check if the arguments to the | ||
| /// process are valid Unicode. To ensure UTF-8 validity, |
There was a problem hiding this comment.
I'm a bit late to the table here, happening to stumble upon this just after it's merged, but I must state that I find both original and updated changes here to be flawed.
Filenames and paths, which of course are sometimes provided as arguments to a program, are not guaranteed to be valid UTF-8 on some platforms/filesystems. Consequently there is a possibility of arguments representing real filenames/paths genuinely being non-UTF-8 strings. Use of args to collect arguments that take filenames/paths would obviously result in a crash for such inputs, blocking users from potentially using real files/paths with that program.
In theory any argument (or env-var), as external input to the program, could potentially be invalid UTF-8, and simply crashing in the face of this, rather than gracefully exiting with a clear error where appropriate, is really not good behaviour.
The benefit of args over args_os is simplicity for cases where for instance your app does not take any filename/path arguments, and you don't care if invalid UTF-8 argument inputs cause a crash. It is great for beginners getting to grips with the basics of playing with argument handling.
The benefit of args_os over args is that it allows you to "correctly" handle all possible filenames/paths for such arguments, and allows you to more gracefully handle situations of otherwise unexpected invalid UTF-8 input, at the expense of the minor complication of having to handle OsString/OsStr types. It could be viewed as the better/right choice for those with more knowledge and experience.
Thus the choice is nothing to do with safety exactly, as already pointed out, but also is not really about "ensuring UTF-8 validity" either. It's about (a) simplicity - avoiding you having to check for and handle non-UTF-8 situations as above, (b) whether or not you care about properly supporting possibly non-UTF-8 filename/path arguments, (c) whether or not you care about ungraceful crashes in the face of non-UTF-8 input otherwise.
This is not what is conveyed by this modified documentation. It might be best, really, for some form of what I have just written to be placed into the documentation to properly inform those trying to make a choice.
There was a problem hiding this comment.
I would like to keep it short so maybe it can be changed to something simple like If you want to panic on invalid UTF-8, use the [`args`] function instead.? This sentence plainly states what happens and doesn't mention "safety" or "validity".
But maybe it could still be nice to have that additional information you provided, so if you want to add that please open a PR. But if you think the sentence I just suggested suffices, I would be happy to make another PR changing that.
There was a problem hiding this comment.
I think that that sentence is certainly much better, but I feel that there is significant value in actually explaining the situation regarding what I wrote above, such that people reading it can make a properly informed choice. I'll try to take a look at further improving the text later. :)
There was a problem hiding this comment.
Actually, we have the exact same case for vars, vars_os and var, var_os too, don't we? I think you are making a really good point and it doesn't just apply to args and args_os so because this is more general I think what you are saying would be a good fit for maybe the docs of std::env? Maybe you could mention those 6 methods at the bottom there and discuss them, rather than copy-paste to every of those 6 methods.
Make sentence in env::args_os' docs plain and simple Follow-up to rust-lang#84551. See rust-lang#84551 (comment) on why this makes more sense.
…iplett
Link between std::env::{var, var_os} and std::env::{vars, vars_os}
In rust-lang#84551 I linked between `std::env::{args, args_os}` and this PR does the same but for `std::env::{var, var_os}` and `std::env::{vars, vars_os}`. Now all of `std::env::{var, var_os, vars, vars_os, args, args_os}` should each mention their `_os` or non-`_os` equivalent in the docs so that you can easily navigate between them.
…iplett
Link between std::env::{var, var_os} and std::env::{vars, vars_os}
In rust-lang#84551 I linked between `std::env::{args, args_os}` and this PR does the same but for `std::env::{var, var_os}` and `std::env::{vars, vars_os}`. Now all of `std::env::{var, var_os, vars, vars_os, args, args_os}` should each mention their `_os` or non-`_os` equivalent in the docs so that you can easily navigate between them.
I noticed that
args_oswas missing some information and I thought it should mentionargsfor when you want more safety just like howargsmentionsargs_osif you don't want it to panic on invalid Unicode.