luhn: tweak canonical tests#547
Conversation
| }, | ||
| { | ||
| "description": "simple valid sin", | ||
| "description": "simple valid SIN", |
There was a problem hiding this comment.
Capitalized sin -> SIN throughout.
| { | ||
| "description": "another valid Canadian SIN", | ||
| "input": "055 444 285", | ||
| "expected": true |
There was a problem hiding this comment.
In the PR that added this test, the following reasoning was given:
Part of it was the contrast with 055-444-285, and part of it was the thought that having a couple of different longer valid numbers might help someone who was struggling to work out how the algorithm needed to be implemented.
I moved it earlier in the suite so that it is more likely encountered by someone who is still trying to work out the algorithm, and updated the language in the test cases that contrast with this one to make that comparison more clear.
| { | ||
| "description": "punctuation is not allowed", | ||
| "description": "valid strings with punctuation included become invalid", | ||
| "input": "055-444-285", |
There was a problem hiding this comment.
Compare to valid case 055 444 285.
| }, | ||
| { | ||
| "description": "symbols are not allowed", | ||
| "description": "valid strings with symbols included become invalid", |
There was a problem hiding this comment.
Compare to valid case 055 444 285.
| { | ||
| "description": "another valid sin", | ||
| "input": "055 444 285", | ||
| "description": "more than a single zero is valid", |
There was a problem hiding this comment.
Updated to state the limitation here more clearly, and moved the space character to indicate that a "chain" of zeros is not required.
| }, | ||
| { | ||
| "description": "nine doubled is nine", | ||
| "description": "input digit 9 is correctly converted to output digit 9", |
There was a problem hiding this comment.
Nine doubled is not nine; tried to make this a little more explicit!
| "description": "another valid sin", | ||
| "input": "055 444 285", | ||
| "description": "more than a single zero is valid", | ||
| "input": "00 000", |
There was a problem hiding this comment.
Maybe "0 0000" would be even better here?
|
I was also tempted to update the description for this test to make it more clear that this is the only test that will fail if the "wrong" set of alternating digits are manipulated. Thoughts on that? |
petertseng
left a comment
petertseng
left a comment
There was a problem hiding this comment.
Good ordering change, good description change. Note that in #522 I was in doubt about whether 055 444 285 should be present at all (it is unlikely any student will come up with an implementation that passes all tests except that one).
I was also tempted to update the description for this test to make it more clear that this is the only test that will fail if the "wrong" set of alternating digits are manipulated
Indeed, I was very inspired by exercism/go#500
If you wanted to go one further to emphasize it, you could try this.
First, we have the case 059. This is a short valid SIN, but since it has an odd number of digits, it doesn't matter whether you double from the left or right.
Then, we have 59 which has an even number of digits to show that it's important to double the right digits.
|
Good call; I'll update that now! |
|
@petertseng updated with a few changes:
|
|
@petertseng see also #549 which I believe addresses another potential source of confusion for implementors. |
|
These changes look good to me. The descriptions and the test cases make sense and there doesn't seem to be enough overlap between them where they could be condensed down without sacrificing something. I don't have merge rights here but 👍 |
|
Great work, thanks! 🎉 |
travis-ci build status badge
4 participants
No description provided.