Improve {BTreeMap,HashMap}::get_key_value docs.
#132758
Conversation
rustbot
added
S-waiting-on-review
| } | ||
|
|
||
| /// Returns the key-value pair corresponding to the supplied key. | ||
| /// Returns the key-value pair corresponding to the supplied key. This is |
There was a problem hiding this comment.
When you index a hashmap, you don't actually use K for indexing, but you use Q that upholds K: Borrow<Q>. So another use-case could be that you want to inspect &K when you only have &Q, right? Like if you have &str as an indexing key for HashMap<String, T>, but you want to inspect e.g. the capacity of the &String key.
There was a problem hiding this comment.
Yeah, I think the K/Q difference would be more interesting to demonstrate.
There was a problem hiding this comment.
Hmm, let me explain how this PR came about. This method has one genuine use in the compiler, in rustc_resolve. (There are three call sites which are all extremely similar.) I had never heard of this method, so I looked up the docs and found them unhelpful. After more digging I discovered that the method was being used in the way I described/demonstrated in this PR: with a type where one of the fields was being ignored.
So while I can see that the &K/&Q case is possible, the very limited real-world experience I have suggests the "ignored field" case is more interesting. "you want to inspect e.g. the capacity of the &String key" feels not that useful.
Also, I just looked through the version control history. The get_key_value methods were added in #49346, implementing the suggestion from #43143. The only justification there is "sometimes you need a reference to a key with the lifetime of the collection", which is a third motivation, but there are no actual examples and surprisingly little discussion :(
So now my inclination is to expand the text description of the methods to include the &K/&Q case and the "reference to a key with the lifetime of the collection" case, but keep the examples on the "ignore a field" case. Thoughts?
There was a problem hiding this comment.
That sounds good to me. I agree that your use-case is probably the most useful one.
They are unusual methods. The docs don't really describe the cases when they might be useful (as opposed to just `get`), and the examples don't demonstrate the interesting cases at all. This commit improves the docs and the examples.
nnethercote
force-pushed
the
improve-get_key_value-docs
branch
from
ea41137 to
2765432
Compare
|
@cuviper: I updated the method descriptions to cover the three cases we have discussed. |
| /// map.insert(j_a, "Paris"); | ||
| /// assert_eq!(map.get_key_value(&j_a), Some((&j_a, &"Paris"))); | ||
| /// assert_eq!(map.get_key_value(&j_b), Some((&j_a, &"Paris"))); // the notable case | ||
| /// assert_eq!(map.get_key_value(&p), None); |
There was a problem hiding this comment.
Muad'Dib knew that every experience carries its lesson.
There was a problem hiding this comment.
Static typing is better than dynamic typing.
- Winston Churchill
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
5 participants
They are unusual methods. The docs don't really describe the cases when they might be useful (as opposed to just
get), and the examples don't demonstrate the interesting cases at all.This commit improves the docs and the examples.