roman-numerals: add descriptions#707
roman-numerals: add descriptions#707petertseng merged 1 commit intoexercism:masterfrom petertseng:roman-desc
Conversation
|
I think it is great, considering how hard it is to to come up with a reason for test cases that where previously written. 👍 |
ErikSchierboom
left a comment
ErikSchierboom
left a comment
There was a problem hiding this comment.
I must say, these descriptions are brilliant!
petertseng
left a comment
petertseng
left a comment
There was a problem hiding this comment.
Well that's enough agreement for me to merge it.
I hope that characters like , and - and + in the descriptions don't mess test generators up.
I have a few questions about the test ordering that can be explored in the future. But maybe we decide that it's the most logical to go in numeric order.
I might have also found it useful to test other potential mistaken subtractions: 49, 45, 99, 95, 499, 495, 490, 450, 999, 995, 990, 950. Why is 49 XLIX rather than IL? Looks like https://www.reddit.com/r/NoStupidQuestions/comments/2tngkh/why_is_49_xlix_instead_of_il_in_the_roman_numeral/ explains (so that if I see an I, then I can be assured that the ensuing number is small in magnitude - this assurance is broken if an I can precede an L)
| "expected": "CXLI" | ||
| }, | ||
| { | ||
| "description": "60, being 50 + 10, is LX", |
There was a problem hiding this comment.
I decided that the only new thing this case was testing was 60, but it's a bit of an illogical place to do that. It can be done earlier (near 50 and 90), without dealing with 100s. But I tried not to change tests in this PR.
| "expected": "III" | ||
| }, | ||
| { | ||
| "description": "4, being 5 - 1, is IV", |
There was a problem hiding this comment.
seems slightly illogical to refer to 4 as 5 - 1 before 5 has been introduced, but maybe someone thinks it's really important to use the natural ordering of numbers.
There was a problem hiding this comment.
Hmmm, I a bit torn on this one. Perhaps we should first do all of the individual letter case (I, V, X, etc.) and then move on to the edge cases? I think that would make more sense for this particular problem domain, even though it can seem a bit odd at first.
| "expected": "VI" | ||
| }, | ||
| { | ||
| "description": "9, being 10 - 1, is IX", |
There was a problem hiding this comment.
same: 9 as 10 - 1 before 10 introduced.
| "expected": "LIX" | ||
| }, | ||
| { | ||
| "description": "90, being 100 - 10, is XC", |
There was a problem hiding this comment.
same: 90 as 100 - 10 before 100 introduced.
| "expected": "CLXIII" | ||
| }, | ||
| { | ||
| "description": "400, being 500 - 100, is CD", |
There was a problem hiding this comment.
same: 400 as 500 - 100 before 500 introduced.
| "expected": "DLXXV" | ||
| }, | ||
| { | ||
| "description": "900, being 1000 - 100, is CM", |
There was a problem hiding this comment.
same: 900 as 1000 - 100 before 1000 introduced.
| }, | ||
| { | ||
| "description": "400, being 500 - 100, is CD", | ||
| "number" : 402, |
There was a problem hiding this comment.
this tests a useful property noted in the README: empty places are skipped. Since the tens place is zero, we don't see an X or L in this answer. It might be useful to call this out. One might see this as distinct from 10 -> X because there are numbers on both sides of the zero.
| }, | ||
| { | ||
| "description": "1000 is a single M", | ||
| "number" : 1024, |
There was a problem hiding this comment.
same, empty 100's place can be called out interestingly.
At the moment we don't restricted the contents of the descriptions. The only thing I expected, which is not enforced, is that it doesn't contain control characters, newlines or non ASCII character. Considering that the |
3 participants
We'd like some descriptions so that roman-numerals can be brought into
compliance with the schema, as in #625.
Closes #451 via replacement