doc: std::env::var: Returns None for names with '=' or NUL byte #128902
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The documentation incorrectly stated that std::env::var could return an error for variable names containing '=' or the NUL byte. Copy the correct documentation from var_os. var_os was fixed in Commit 8a7a665, Pull Request rust-lang#109894, which closed Issue rust-lang#109893. This documentation was incorrectly added in commit f2c0f29, which replaced a panic in var_os by returning None, but documented the change as "May error if ...". Reference the specific error values and link to them.
|
rustbot has assigned @workingjubilee. Use |
rustbot
added
S-waiting-on-review
library/std/src/env.rs
Outdated
Comment on lines
201
to
206
| /// This function returns [`VarError::NotPresent`] if the environment variable | ||
| /// isn't set. | ||
| /// | ||
| /// This function may return an error if the environment variable's name contains | ||
| /// the equal sign character (`=`) or the NUL character. | ||
| /// This function may return [`VarError::NotPresent`] if the | ||
| /// environment variable's name contains the equal sign character (`=`) or the | ||
| /// NUL character. |
There was a problem hiding this comment.
can we frame this in a way that visibly ORs these two together? like
Suggested change
| /// This function returns [`VarError::NotPresent`] if the environment variable | |
| /// isn't set. | |
| /// | |
| /// This function may return an error if the environment variable's name contains | |
| /// the equal sign character (`=`) or the NUL character. | |
| /// This function may return [`VarError::NotPresent`] if the | |
| /// environment variable's name contains the equal sign character (`=`) or the | |
| /// NUL character. | |
| /// This function may return [`VarError::NotPresent`] if | |
| /// - the environment variable isn't set | |
| /// - the environment variable's name contains an equals sign or NUL (`=` or `\0`) |
There was a problem hiding this comment.
I suppose we could leave it as you have written it since it's copying directly from var_os for the most part, but it feels strange to me that they are being made to match here but one has an # Errors header?
There doesn't seem to be, even in this module, consistency in whether that is present on Result-returning functions, or what it documents:
- https://doc.rust-lang.org/std/env/fn.set_current_dir.html
- https://doc.rust-lang.org/std/env/fn.current_exe.html
- https://doc.rust-lang.org/std/env/fn.current_dir.html
It doesn't seem to make things clearer the way the # Examples header tends to.
There was a problem hiding this comment.
The main reason I bring this up is that for the most part they are self-explanatory variants except the "returns NotPresent if we determine the variable is invalid" happens yes, but is an essentially spurious error, and Option only requires spelling this out because it doesn't attach a direct semantic to the Some or None variants, being a much more "vibe-based" notion of "was available".
This PR description even says None instead of NotPresent, here.
There was a problem hiding this comment.
Excellent suggestions, thanks! I have pushed a new revision:
- Uses a bullet list for the error results. This is much clearer, thanks! This makes it look similar to current_dir.
- Minor tweaks to make the text a bit shorter.
I have left the Errors header, but if you want it removed, I'm happy to do that also. I think the general recommendation is that functions returning a Result should document the errors in an Errors section. Some evidence:
- The API guide recommends it : "Error conditions should be documented in an "Errors" section"
- Clippy's pedantic lint that warns about functions using a Result without an Errors section: https://rust-lang.github.io/rust-clippy/master/#/missing_errors_doc
- In the
std::envmodule,current_exe,current_dir,join_paths, andvarcurrently all include an Errors section. The only function that returns a Result without an Errors section isset_current_dir. I can submit a separate change to add an Errors section for that function as well.
Current rendered version:
There was a problem hiding this comment.
Ah, I see, so set_current_dir is the odd one out. That's fine then!
Yes, that looks nicer. Thank you!
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Aug 18, 2024
…rkingjubilee doc: std::env::var: Returns None for names with '=' or NUL byte The documentation incorrectly stated that std::env::var could return an error for variable names containing '=' or the NUL byte. Copy the correct documentation from var_os. var_os was fixed in Commit 8a7a665, Pull Request rust-lang#109894, which closed Issue rust-lang#109893. This documentation was incorrectly added in commit f2c0f29, which replaced a panic in var_os by returning None, but documented the change as "May error if ...". Reference the specific error values and link to them.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 18, 2024
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#125854 (Move ZST ABI handling to `rustc_target`) - rust-lang#127623 (fix: fs::remove_dir_all: treat internal ENOENT as success) - rust-lang#128084 (Suggest adding Result return type for associated method in E0277.) - rust-lang#128902 (doc: std::env::var: Returns None for names with '=' or NUL byte) - rust-lang#129187 (bootstrap: fix clean's remove_dir_all implementation) - rust-lang#129235 (Check that `#[may_dangle]` is properly applied) - rust-lang#129245 (Typo) r? `@ghost` `@rustbot` modify labels: rollup
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Aug 18, 2024
…rkingjubilee doc: std::env::var: Returns None for names with '=' or NUL byte The documentation incorrectly stated that std::env::var could return an error for variable names containing '=' or the NUL byte. Copy the correct documentation from var_os. var_os was fixed in Commit 8a7a665, Pull Request rust-lang#109894, which closed Issue rust-lang#109893. This documentation was incorrectly added in commit f2c0f29, which replaced a panic in var_os by returning None, but documented the change as "May error if ...". Reference the specific error values and link to them.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 19, 2024
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#128084 (Suggest adding Result return type for associated method in E0277.) - rust-lang#128902 (doc: std::env::var: Returns None for names with '=' or NUL byte) - rust-lang#129187 (bootstrap: fix clean's remove_dir_all implementation) - rust-lang#129194 (Fix bootstrap test `detect_src_and_out` on Windows) - rust-lang#129231 (improve submodule updates) - rust-lang#129235 (Check that `#[may_dangle]` is properly applied) - rust-lang#129245 (Typo) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 19, 2024
Rollup of 7 pull requests Successful merges: - rust-lang#127679 (Stabilize `raw_ref_op` (RFC 2582)) - rust-lang#128084 (Suggest adding Result return type for associated method in E0277.) - rust-lang#128628 (When deduplicating unreachable blocks, erase the source information.) - rust-lang#128902 (doc: std::env::var: Returns None for names with '=' or NUL byte) - rust-lang#129048 (Update `crosstool-ng` for loongarch64) - rust-lang#129116 (Include a copy of `compiler-rt` source in the `download-ci-llvm` tarball) - rust-lang#129208 (Fix order of normalization and recursion in const folding.) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 19, 2024
Rollup merge of rust-lang#128902 - evanj:evan.jones/env-var-doc, r=workingjubilee doc: std::env::var: Returns None for names with '=' or NUL byte The documentation incorrectly stated that std::env::var could return an error for variable names containing '=' or the NUL byte. Copy the correct documentation from var_os. var_os was fixed in Commit 8a7a665, Pull Request rust-lang#109894, which closed Issue rust-lang#109893. This documentation was incorrectly added in commit f2c0f29, which replaced a panic in var_os by returning None, but documented the change as "May error if ...". Reference the specific error values and link to them.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The documentation incorrectly stated that std::env::var could return an error for variable names containing '=' or the NUL byte. Copy the correct documentation from var_os.
var_os was fixed in Commit 8a7a665, Pull Request #109894, which closed Issue #109893.
This documentation was incorrectly added in commit f2c0f29, which replaced a panic in var_os by returning None, but documented the change as "May error if ...".
Reference the specific error values and link to them.