Just opened the PR to add the exercise for LFE: Add Roman numerals practice exercise by kahgoh · Pull Request #188 · exercism/lfe · GitHub
Hi! I was just doing this exercise as part of the challenge (started a bit late), and noticed a test gap: the longest possible output (3888 → MMMDCCCLXXXVIII) is not tested. This is important for languages like C where you have to allocate a large enough buffer, so IMO is should be tested. (Though scrolling through community solutions it looks like most got it right.)
I’m not familiar enough with how exercism works to open a PR adding such a test (especially as I suspect tests for individual languages are somehow generated from a list shared between all the tracks that have this exercise?) so I thought I’d mention it here hoping that someone could add this test.
Thanks in advance!
Edited to add: the Python “dig deeper” page says “the longest string needed to represent a Roman numeral is 14 characters (MMDCCCLXXXVIII)” but I think it should be “15 characters (MMMDCCCLXXXVIII)”.
Yes, that looks like my mistake. Would you like to submit a PR for this? If not, I’m happy to correct it.
problem-specifications/exercises at main · exercism/problem-specifications · GitHub contains the canonical data from which tracks should source their tests when porting a specific practice exercise like roman-numerals. Contributing there means more tracks can see and discuss your proposed edits, but some tracks also implement their own tests alongside the canonical ones. What you’re proposing might be more appropriate as a track-specific test since the length of the returned string wouldn’t be an issue on any of the tracks I maintain.
Thanks for the pointer! That’s the main thing I was missing :)
In doubt, I’ve created a PR on problem-specifications with the following reasoning:
- If/when this exercise is added to a language with manual memory management in the future, that way it’ll automatically get that test.
- The documentation (thanks @IsaacG for the link!) seems to encourage that.
However if it turns out that was misguided and this PR is rejected, I’m happy to raise a PR on the C repo instead.
PR merged.
PR Merged.