-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add doc examples for OsStr
, OsString
.
#40458
Conversation
9985c8f
to
50903b5
Compare
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.
Just one nit.
src/libstd/ffi/os_str.rs
Outdated
/// | ||
/// # Examples | ||
/// | ||
/// ```rust |
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.
The rust annotation is useless.
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.
The API documentation conventions don't mention that we have to remove these annotations.
https://github.com/rust-lang/rfcs/blob/master/text/1574-more-api-documentation-conventions.md
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 removed the annotations in a lot of places, so please don't add them back.
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.
Latest force push removes them.
I removed the annotations in a lot of places, so please don't add them back.
Sure, but just because you removed the annotations doesn't mean we should enforce this everywhere. Personally, I prefer to leave in the rust
annotation.
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.
Might be worth to start an official statement about this with @rust-lang/docs.
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 thought that the RFC stated to not use them more strongly, but apparently it does not.
src/libstd/ffi/os_str.rs
Outdated
/// use std::ffi::OsString; | ||
/// | ||
/// let mut s = OsString::new(); | ||
/// s.reserve_exact(10); |
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.
reserve_exact
-> reserve
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.
Good catches, thanks!
src/libstd/ffi/os_str.rs
Outdated
/// use std::ffi::OsString; | ||
/// | ||
/// let mut s = OsString::new(); | ||
/// s.reserve(10); |
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.
reserve
-> reserve_exact
50903b5
to
a31c6d4
Compare
Comments have been addressed. |
a31c6d4
to
6adbbfc
Compare
@GuillaumeGomez Everything look good here? |
☔ The latest upstream changes (presumably #40009) made this pull request unmergeable. Please resolve the merge conflicts. |
r=me once conflicts fixed. |
@bors r=GuillaumeGomez rollup |
📌 Commit bc6eecd has been approved by |
…eGomez Add doc examples for `OsStr`, `OsString`. None
…eGomez Add doc examples for `OsStr`, `OsString`. None
…eGomez Add doc examples for `OsStr`, `OsString`. None
No description provided.