Skip to content
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.

Sign up for GitHub

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

Jump to bottom

doc: std::env::var: Returns None for names with '=' or NUL byte #128902

Merged
merged 2 commits into from
Aug 19, 2024
Merged

doc: std::env::var: Returns None for names with '=' or NUL byte #128902

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d5a7c45
doc: std::env::var: Returns None for names with '=' or NUL byte
evanj Aug 9, 2024
b0023f5
code review improvements
evanj Aug 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions library/std/src/env.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,13 +198,16 @@ impl fmt::Debug for VarsOs {
///
/// # Errors
///
/// This function will return an error if the environment variable isn't set.
/// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

errors header creating this additional visual distinction

There doesn't seem to be, even in this module, consistency in whether that is present on Result-returning functions, or what it documents:

It doesn't seem to make things clearer the way the # Examples header tends to.

Copy link
Member

@workingjubilee workingjubilee Aug 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::env module, current_exe, current_dir, join_paths, and var currently all include an Errors section. The only function that returns a Result without an Errors section is set_current_dir. I can submit a separate change to add an Errors section for that function as well.

Current rendered version:

errors

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, so set_current_dir is the odd one out. That's fine then!

Yes, that looks nicer. Thank you!

///
/// This function will return an error if the environment variable's value is
/// not valid Unicode. If this is not desired, consider using [`var_os`].
/// This function will return [`VarError::NotUnicode`] if the environment
/// variable's value is not valid Unicode. If this is not desired, consider
/// using [`var_os`].
///
/// # Examples
///
Expand Down
Loading