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

feat: Rewrite try_from_into #393

Merged
merged 5 commits into from
May 15, 2020
Merged

feat: Rewrite try_from_into #393

merged 5 commits into from
May 15, 2020

Conversation

ikar49
Copy link
Contributor

@ikar49 ikar49 commented May 12, 2020

English is not my native language, so I may have made some mistakes in the text. I would be grateful if someone could check it out.

closes #392

Instead of working with strings, the exercise suggests creating a Color structure from other simple types (tuple, array, slice).

closes #392
@AbdouSeck
Copy link
Contributor

Hi @ikar49,

This looks really good. Thank you for the great work. I have one suggestion here. The hint provided in the info.toml for the from_str exercise tells people to do an almost copy-paste operation from the try_from_into exercise to answer the question. However, that will not be the case once this pull request gets merged.
Can you please edit the hint for from_str to say something else? You can say something like:

The implementation of FromStr should return an Ok with a Person object, or an Err with a string if the string is not valid.

Please feel free to reach out if you have any questions.

Thank you so much!

Abdou

Copy link
Contributor

@AbdouSeck AbdouSeck left a comment

Choose a reason for hiding this comment

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

Hi @ikar49,

Sorry, I completely overlooked something:

I realized the Color is defined with u8 as inner types while the implementations are asking for i16. While this may not be a problem by itself, if someone directly converts an i16 to a u8, there may be some wrapping issues. For instance, the following will compile, but is not really what you would want:

fn main() {
    let v = 1000;
    println!("{}", v as u8);
}

Evidently, this can be resolved by adding unit tests that makes sure that if a value greater than 255 is provided, then an error is returned.

Please let me know if you have a question about this.

Thank you,

Abdou

@ikar49
Copy link
Contributor Author

ikar49 commented May 15, 2020

Hello @AbdouSeck.

Are you suggesting replacing i16 with isize, if I understand correctly?

@AbdouSeck
Copy link
Contributor

@ikar49,

Not really. I think it would be fine to just add a test cases to handle when values above 255 are passed. I am thinking of the following tests:

#[test]
fn test_out_of_range_slice() {
    let v = vec![10000, 1000, 1000];
    let c = Color::try_from(&v[..]);
    assert!(c.iserr());
}

and

#[test]
fn test_out_of_range_array() {
    let v = [10000, 1000, 1000];
    let c = Color::try_from(v);
    assert!(c.iserr());
}

and also

#[test]
fn test_out_of_range_tuple() {
    let v = (10000, 1000, 1000);
    let c = Color::try_from(v);
    assert!(c.iserr());
}

These will make sure that the implementations remember to return an error if the values provided are greater than 255.

Please let me know if you have any questions.

Thank you,

Abdou

@ikar49
Copy link
Contributor Author

ikar49 commented May 15, 2020

@AbdouSeck,

I fixed it. What you think about it?

Copy link
Contributor

@AbdouSeck AbdouSeck left a comment

Choose a reason for hiding this comment

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

Hi @ikar49, I think this should take care of all types of out-of-range issues. Thank you so much!

@AbdouSeck AbdouSeck added A-exercises Area: Exercises C-enhancement Category: Enhancement labels May 15, 2020
Copy link
Member

@shadows-withal shadows-withal left a comment

Choose a reason for hiding this comment

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

Thank you so much!

@shadows-withal shadows-withal changed the title Rewrite try_from_into feat: Rewrite try_from_into May 15, 2020
@shadows-withal shadows-withal merged commit 763aa6e into rust-lang:master May 15, 2020
MendelMonteiro pushed a commit to MendelMonteiro/rustlings that referenced this pull request Jun 28, 2020
dlemel8 pushed a commit to dlemel8/rustlings that referenced this pull request Aug 2, 2020
@ekaats
Copy link

ekaats commented Apr 30, 2021

I've just been going through the rustlings exercises and found this one a little confusing. I could not get the type Error = Box<dyn error::Error>; working. After changing this back to type Error = String; and just returning an Err("Something went wrong")-esque error I was able to finish the exercise without issue. Maybe make a separate exercise for that error?

ppp3 pushed a commit to ppp3/rustlings that referenced this pull request May 23, 2022
dmoore04 pushed a commit to dmoore04/rustlings that referenced this pull request Sep 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-exercises Area: Exercises C-enhancement Category: Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Solutions for try_from_into and from_str are same
4 participants