rustlings icon indicating copy to clipboard operation
rustlings copied to clipboard

fix: Add a deref in the test code

Open aaarkid opened this issue 3 years ago • 1 comments

This change makes it possible to finish the last exercise without touching the test code, seems like an overlook when code was added since there's nothing in the hints about this extension neither.

It's virtually impossible to write a the num_sq function to take the Box since it doesn't implement MulAssign.

aaarkid avatar Sep 09 '22 00:09 aaarkid

I believe this PR is also a good moment to add something in the hints regard the AsMut part.

aaarkid avatar Sep 09 '22 00:09 aaarkid

Can this PR be merged? It only changes 1 line and saves people finishing Rustlings a good bunch of minutes up to an hour.

aaarkid avatar Oct 16 '22 17:10 aaarkid

@all-contributors please add @aaarkid for content

manyinsects avatar Oct 18 '22 09:10 manyinsects

@diannasoreil

I've put up a pull request to add @aaarkid! :tada:

allcontributors[bot] avatar Oct 18 '22 09:10 allcontributors[bot]

This change makes it possible to finish the last exercise without touching the test code

I'm surprised at this PR as now I can't implement the function without changing the test code.

This was my implementation
fn num_sq<T: AsMut<u32>>(mut arg: T) {
    let arg = arg.as_mut();
    *arg *= *arg;
}

I have to remove the * added by this PR to get it to compile.

@aaarkid can you share your solution?

shepmaster avatar Oct 19 '22 17:10 shepmaster

Sure, in a bit.

aaarkid avatar Oct 19 '22 18:10 aaarkid

I also can't get it to compile in the "obvious" way, and it certainly works without the * in the test code.

Another argument in favor of reverting this - the example right in the AsMut documentation shows it working without the *: https://doc.rust-lang.org/std/convert/trait.AsMut.html#examples

It's virtually impossible to write a the num_sq function to take the Box since it doesn't implement MulAssign.

It doesn't need to, isn't that the point of AsMut? You only need MulAssign for &mut u32, because that's what (&mut my_box_u32).as_mut() will give you here.

seritools avatar Oct 19 '22 21:10 seritools

I see the issue. You're right the deref shouldn't be there. I'll open a new PR tomorrow to revert this. I apologize for the trouble this may have caused.

In the meanwhile, do you think that the let arg = arg.as_mut(); should already be in the code?

aaarkid avatar Oct 19 '22 22:10 aaarkid

In the meanwhile, do you think that the let arg = arg.as_mut(); should already be in the code?

I'm not the target audience of Rustlings, but my gut says "no, it should not be there".

shepmaster avatar Oct 20 '22 17:10 shepmaster

My CI fails because of this change https://github.com/azzamsa/rustlings/actions/runs/3317605384/jobs/5480649114#step:6:12

azzamsa avatar Oct 25 '22 02:10 azzamsa