-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Various improvements in std::collections docs #41286
Conversation
This greatly improves consistency.
* Added links where possible (limited because of facading) * Changed references to methods from `foo()` to `foo` in module docs * Changed references to methods from `HashMap::foo` to just `foo` in top-level docs for `HashMap` and the `default` doc for `DefaultHasher` * Various small other fixes
* Changed btree_map's and hash_map's Entry (etc.) docs to be consistent * Changed VecDeque's type and module summary sentences to be consistent with each other as well as with other summary sentences in the module * Changed HashMap's and HashSet's summary sentences to be less redundantly phrased and also more consistant with the other summary sentences in the module * Also, added an example to Bound
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.
This is so great, thanks so much! Just a couple comments
src/libcollections/binary_heap.rs
Outdated
@@ -218,10 +219,14 @@ pub struct BinaryHeap<T> { | |||
data: Vec<T>, | |||
} | |||
|
|||
/// A container object that represents the result of the [`peek_mut`] method | |||
/// on `BinaryHeap`. See its documentation for details. | |||
/// Object representing a mutable reference to the greatest item on a |
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 think we try to avoid using the term 'object' to describe structures since Rust isn't really OO. Personally I use 'structure', though there might be a better word.
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.
Maybe "Structure wrapping a mutable reference..."?
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.
Yeah, that sounds better!
/// map.insert(5, "b"); | ||
/// map.insert(8, "c"); | ||
/// | ||
/// for (key, value) in map.range((Excluded(3), Included(8))) { |
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 think you should be able to use the Range
sugar here: 4..9
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.
Yeah, that's true. I adapted this example from the one for BTreeMap::range
, which I agree is not ideal; as far as I could tell those kinds of methods are the only place in the (stable) standard library where Bound
can be used right now though. I'd rather use something like the examples in RangeArgument
, but that's not stable yet so I wasn't sure if that is ok to use.
Linkchecker is failing on the links I added to Should I add it to the whitelist, or should I remove the links there again (and in other places too)? I feel like the docs are better with the links there in the version directly in |
Yeah, it's a bit of an unfortunate situation right now with docs links, though hopefully it will be fixed in the future. Either whitelisting or removing the links is fine with me. Do whichever your prefer! |
src/libcollections/lib.rs
Outdated
@@ -135,6 +135,23 @@ mod std { | |||
} | |||
|
|||
/// An endpoint of a range of keys. | |||
/// # Examples |
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.
there should be an extra ///
above this line
* Bound: * Added another example using RangeArgument to illustrate how Bound maps to range endpoints. * Added a note to the existing example that says that it's better to use range syntax in most cases * Added missing /// line * binary_heap::PeakMut: s/Object representing/Structure wrapping * added collections/hash_set/struct.HashSet.html to linkchecker whitelist
I went with adding After talking about it on IRC, I also added a second example for |
Wooo thanks! ✨ @bors r+ rollup |
📌 Commit 2a23e6e has been approved by |
Various improvements in std::collections docs The meat of this PR are: * changes to (almost all?) iterator struct docs in std::collections such that they use the standard iterator boilerplate and state where they are created * a bunch of added links (at least as much as possible given std::collections mostly being a facade and whatnot 😅) * an example for `Bound` * changed phrasing for some summary sentences to be less redundant as well as more consistant with others in the module There also are various other fixes, e.g. removing parens from method names in the module docs, changing some imperatives to 3rd person, etc. r? @steveklabnik
☀️ Test successful - status-appveyor, status-travis |
The meat of this PR are:
Bound
There also are various other fixes, e.g. removing parens from method names in the module docs, changing some imperatives to 3rd person, etc.
r? @steveklabnik