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.

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

Improve hint for as_ref_mut #1113

Merged
merged 1 commit into from
Jun 12, 2023
Merged

Improve hint for as_ref_mut #1113

merged 1 commit into from
Jun 12, 2023

Conversation

Frosthage
Copy link

The hint for as_ref_mut was written before the function num_sq was added and is currently incorrect. I'm pretty sure you can improve this hint, but at least it's correct

@@ -1135,4 +1135,4 @@ name = "as_ref_mut"
path = "exercises/conversions/as_ref_mut.rs"
mode = "test"
hint = """
Add AsRef<str> as a trait bound to the functions."""
Add AsRef<str> or AsMut<u32> as a trait bound to the functions."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Add AsRef<str> or AsMut<u32> as a trait bound to the functions."""
Add AsRef<str> or AsMut<u32> as a trait bound to the functions as appropriate."""

Copy link

@CaliViking CaliViking Aug 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you limit the trait to u32? Shouldn't the trait allow any parameter that can be squared, so Mul trait? I know that the unit test specifies u32, but that seems pretty narrow, shouldn't the function support i32, i64, u32, u64, f32, f64?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just as an example 🤷

Copy link

@godmir godmir Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can actually make it generic, e.g.:

fn num_sq<T: AsMut<U> + AsRef<U>, U>(arg: &mut T) where U: Copy + MulAssign<U> {
    let x = *arg.as_ref();
    *arg.as_mut() *= x;
}

@Ttibsi
Copy link

Ttibsi commented Jan 17, 2023

Just realised this existed and went to put in my own PR instead (That I've now closed) - @Frosthage Instead of straight up adding the answer there (and also limiting it to a u32, like @CaliViking mentioned above), what if it had it's own hint saying something like "Consider the type you'd want to use" - I think it's already clear (ish) from the actual comments on that file that it wants to be T: AsMut<_> like the others are something like T: AsRef<str> and the hint just needs to steer the user towards the correct data type. I'm not sure what the data type we could hint towards should be - I used u32 in my own solution, but any numeric one would be fine (does this pass with a generic usize? I've not tested that)

@njr0
Copy link

njr0 commented Mar 3, 2023

FWIW, I just went through all the exercises (as a genuine newbie to Rust) and this was the only exercise I failed to complete properly (as far as I can tell), and I found the hint quite unhelpful.

I could fix it if I specialised num_sq to u32 (with or without the Box), but trying to do with with a combination of generic and Box seemed hard, especially since a lot of types don't implement multiplication. I'd love to see what a correct solution with the generic, handling the Box.

@0xVitalii
Copy link

Great improvement for sure!

@shadows-withal shadows-withal merged commit 06ddcfb into rust-lang:main Jun 12, 2023
@shadows-withal
Copy link
Member

@all-contributors please add @Frosthage for content

@allcontributors
Copy link
Contributor

@shadows-withal

I've put up a pull request to add @Frosthage! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants