Implement exercise isbn-verifier#378
Conversation
|
Hi @ktomsic! Thanks for this submission. At first glance this looks good. I'm going to come back tomorrow to take a real look at it, but I don't anticipate any issues. Assuming I don't discover any, I'll approve this, wait a week for other people to chime in as desired, and then merge it. |
coriolinus
left a comment
coriolinus
left a comment
There was a problem hiding this comment.
Looks good to me! I'm going to wait in order to give other people the chance to chime in with their impressions, but unless someone discovers an issue I missed, I'll merge this on Friday.
petertseng
left a comment
petertseng
left a comment
There was a problem hiding this comment.
thanks for the work.
Readme is same as what's generated. ✔️
Tests match problem-specifications 1.0.0 ✔️
Okay, in that case the only thing I would change is the UUID.
| "loop" | ||
| ] | ||
| }, | ||
| { |
There was a problem hiding this comment.
the last portion is too long (should be 12 hexadecimal digits but has 19 by my count), so this is not a valid https://en.wikipedia.org/wiki/Universally_unique_identifier (I am assuming this is what a UUID is)
There was a problem hiding this comment.
Sorry. I generated this with configlet, but I hadn't seen that it might not generate valid UUIDs. This should be fixed now.
ktomsic
force-pushed
the
isbn-verifier
branch
from
22da320 to
7754abe
Compare
petertseng
left a comment
petertseng
left a comment
There was a problem hiding this comment.
So, I have a favour to ask.
I was dissatisfied with two test cases, and I made exercism/problem-specifications#993 and it got merged.
So, my favour to ask is can you update the test cases and version to match? I have labeled the places that I believe should change.
A separate commit or squashing are both acceptable.
I believe the example solution will already do the right thing for these inputs, so all that needs to be done is to update the tests.
| #[ignore] | ||
| #[allow(non_snake_case)] | ||
| fn test_invalid_isbn_with_invalid_X() { | ||
| assert!(!is_valid_isbn("3-598-2X507-0")); |
There was a problem hiding this comment.
change the last digit 0 to a 9
| #[test] | ||
| #[ignore] | ||
| fn test_invalid_isbn_too_long() { | ||
| assert!(!is_valid_isbn("3-598-21507-XA")); |
| @@ -0,0 +1,4 @@ | |||
| [root] | |||
| name = "isbn-verifier" | |||
| version = "1.0.0" | |||
| @@ -0,0 +1,3 @@ | |||
| [package] | |||
| name = "isbn-verifier" | |||
| version = "1.0.0" | |||
petertseng
left a comment
petertseng
left a comment
There was a problem hiding this comment.
may optionally omit return + semicolon.
| /// Checks if an 'X' is valid at the given position for the given ISBN type | ||
| #[allow(non_snake_case)] | ||
| fn is_X_valid(position: &usize, isbn_type: &IsbnType) -> bool { | ||
| return (isbn_type == &IsbnType::Isbn10 && position == &9 ) || |
There was a problem hiding this comment.
optionally, you may omit the return here if you also omit the trailing semicolon. Up to you whether to do that.
|
|
||
| /// Checks if a '-' is valid at the given position for the given ISBN type | ||
| fn is_dash_valid(position: &usize, isbn_type: &IsbnType) -> bool { | ||
| return isbn_type == &IsbnType::Isbn13 && |
There was a problem hiding this comment.
optionally, you may omit the return here if you also omit the trailing semicolon. Up to you whether to do that.
| coefficient -= 1; | ||
| } | ||
|
|
||
| return checksum % 11 == 0; |
There was a problem hiding this comment.
optionally, you may omit the return here if you also omit the trailing semicolon. Up to you whether to do that.
|
Going to delay merge until @ktomsic gives a definite yes/no to the requests from @petertseng, primarily #378 (review). |
ktomsic
force-pushed
the
isbn-verifier
branch
from
7754abe to
ee04f6d
Compare
|
Sorry. Just been busy the past couple of days. Thanks for the suggestions, @petertseng. I've updated everything accordingly. |
|
Thanks for this contribution, @ktomsic! |
3 participants
No description provided.