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

Example is missing a generic #23

Open
tatupesonen opened this issue Jun 14, 2022 · 3 comments · May be fixed by #49
Open

Example is missing a generic #23

tatupesonen opened this issue Jun 14, 2022 · 3 comments · May be fixed by #49

Comments

@tatupesonen
Copy link

tatupesonen commented Jun 14, 2022 •
edited
Loading

Hello! I'm looking at https://highassurance.rs/chp7/traits.html#the-map-get-api.
The example with the get is never introducing the second generics K and V in the code example.

/// Returns a reference to the value corresponding to the key.
pub fn get<Q>(&self, key: &Q) -> Option<&V>
where
    K: Borrow<Q> + Ord,
    Q: Ord + ?Sized,
{
    // ...function body here...
}

I think a better example would be:

impl<K, V> BTreeMap<K, V> {
    pub fn get<Q>(&self, key: &Q) -> Option<&V>
    where
        K: Borrow<Q> + Ord,
        Q: Ord + ?Sized,
    {
        // ...function body here...
    }
    // ... rest of the methods
}

to show where the generic for BTreeMap are introduced. There is a mention of BTreeMap<K, V> but in my opinion I think it's easily missed and this would illustrate how the generics from the impl are used.
@Skarlett
Copy link

While I like including explicit information,

This information is already stated a line previously.

Based on the above, you might expect the get method's signature to look like this for BTreeMap<K, V>:

furthermore, it distracts from the objective of the article.
I would like to mention that this article isn't a tutorial for building a BTreeMap (otherwise, direct copy and paste would be desirable), but rather an explanation of why BTreeMap uses more than just Ord as a constraint.

@tnballo
Copy link
Owner

tnballo commented Jun 15, 2022

Hey all! Thank you both for looking into this.

Indeed, the text mentions BTreeMap<K, V> and the focus is on why Ord alone doesn't quite get us everything we'd want. So these snippets omit the K and V for brevity.

However, I agree the suggested change would be an improvement! Having both the current text and impl<K, V> BTreeMap<K, V> { ... } code blocks is likely the most clear: Q stands out more as being introduced by fn get<Q> and missing from the impl<K, V> scope (as pointed out). And in this paragraph we're still talking about BTreeMap, haven't gotten to scapegoat trees yet!

I read this really great blog post once about writing technical books. Can't find the original link (will share if I find it later) but the advice was something like "always go for clear over clever, if it's not clear then it can't be clever". Here omitting K and V is "clever" but makes things less "clear". 😉

How about making the suggested change to the first 2 code blocks containing fn get in this section? Would you like to PR this update?

IMO the last block containing fn get, which adds Default to the signature, is safe to leave as is because by that point the reader will be aware of the surrounding impl - having seen it twice before.

@tatupesonen
Copy link
Author

tatupesonen commented Sep 17, 2022 •
edited
Loading

Hey, sorry for the super late reply. I will look into creating a PR for this today!

@pixelshot91 pixelshot91 linked a pull request Feb 22, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chp7: rust_snippet: show type parameter declaration for 'fn get' pixelshot91/high-assurance-rust
3 participants
@Skarlett @tatupesonen @tnballo