Add doc links to std::os extension traits
#49829
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
src/libstd/sys/windows/ext/fs.rs
Outdated
| @@ -18,9 +18,9 @@ use path::Path; | |||
| use sys; | |||
| use sys_common::{AsInnerMut, AsInner}; | |||
|
|
|||
| /// Windows-specific extensions to [`File`]. | |||
| /// Windows-specific extensions to [`fs::File`]. | |||
There was a problem hiding this comment.
We have decided that we qualify every type with the module name?
There was a problem hiding this comment.
Yeah, this isn't something we normally do; the convention is to use the module name for functions (thread::spawn) but not usually types (File). Could we do that here, please?
There was a problem hiding this comment.
Sorry, I wasn't aware. What about types like fs::Metadata, fs::OpenOptions, comand::ExitStatus? These seem like they should be qualified. OsStr and OsString definitely shouldn't be.
There was a problem hiding this comment.
It's all good! I think leaving the namespace when it makes sense for clarity is fine as well, and I think all five examples you provided are correct.
There was a problem hiding this comment.
What about process::Command and fs::DirBuilder?
There was a problem hiding this comment.
I think both of those are borderline cases; I think it's fine to namespace them.
| // knew! | ||
| // | ||
| // As a result to make sure this compiles for all platforms we do the manual | ||
| // casts and rely on manual lowering to `stat` if the raw type is desired. |
There was a problem hiding this comment.
this change feels different than the others? If it's accurate, that's fine, but i wasn't sure.
There was a problem hiding this comment.
This comment was on the trait definition instead of the implementation (I assume that MetadataExt at one time had default implementations for all its methods). Sorry, I struggle with keeping the scope of my PRs limited.
src/libstd/sys/windows/ext/fs.rs
Outdated
| @@ -18,9 +18,9 @@ use path::Path; | |||
| use sys; | |||
| use sys_common::{AsInnerMut, AsInner}; | |||
|
|
|||
| /// Windows-specific extensions to [`File`]. | |||
| /// Windows-specific extensions to [`fs::File`]. | |||
There was a problem hiding this comment.
Yeah, this isn't something we normally do; the convention is to use the module name for functions (thread::spawn) but not usually types (File). Could we do that here, please?
Add documentation links to the original type for various OS-specific extension traits and normalize the language for introducing such traits. Also, remove some outdated comments around the extension trait definitions.
|
Ping from triage! Can @steveklabnik (or someone else from @rust-lang/docs) review this? |
|
@bors: r+ sorry about taking a while here. Thanks! |
|
📌 Commit d5bee64 has been approved by |
bors
added
S-waiting-on-bors
|
@bors: rollup |
Add doc links to `std::os` extension traits Addresses a small subset of rust-lang#29367. This adds documentation links to the original type for various OS-specific extension traits, and uses a common sentence for introducing such traits (which now consistently ends in a period).
Rollup of 11 pull requests Successful merges: - #49461 (std: Child::kill() returns error if process has already exited) - #49727 (Add Cell::update) - #49812 (Fix revision support for UI tests.) - #49829 (Add doc links to `std::os` extension traits) - #49906 (Stabilize `std::hint::unreachable_unchecked`.) - #49970 (Deprecate Read::chars and char::decode_utf8) - #49985 (don't see issue #0) - #50118 (fix search bar bug) - #50139 (encourage descriptive issue titles) - #50174 (Use FxHashMap in syntax_pos::symbol::Interner::intern.) - #50185 (core: Fix overflow in `int::mod_euc` when `self < 0 && rhs == MIN`) Failed merges:
Addresses a small subset of #29367.
This adds documentation links to the original type for various OS-specific extension traits, and uses a common sentence for introducing such traits (which now consistently ends in a period).