-
-
Notifications
You must be signed in to change notification settings - Fork 186
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
phone-number tests dont check for length greater than 11 #124
Comments
We cannot actually say how many digits are allowed in phone numbers , it differs from country to country , there are even 11 to 14 digit phone numbers. |
@deathsec I agree, but the purpose of the exercise isn't to be a 100% accurate, production-ready phone number system. The tests should come from here: https://github.com/exercism/x-common/blob/master/exercises/phone-number/canonical-data.json. If you think there are gaps in the exercise that can be corrected, the place to do it is in the |
then it's missing a few tests (and has some tests that aren't listed at all in xcommon) {
"description": "invalid when 12 digits",
"phrase": "321234567890",
"expected": null
},
{
"description": "invalid with letters",
"phrase": "123-abc-7890",
"expected": null
},
{
"description": "invalid with punctuations",
"phrase": "123-@:!-7890",
"expected": null
},
{
"description": "invalid with right number of digits but letters mixed in",
"phrase": "1a2b3c4d5e6f7g8h9i0j",
"expected": null
} The first in the above checks for one of the cases greater than 11 (i.e. 12) but not all. I'll raise an issue for that there, but there remains the fact that the tests for the xc execrcise differs quite a bit from the xcommon spec shown above. |
Agreed. We should fix everything up when we address this issue. |
The change to x-common repo is made in #719. the test case now reads: {
"description": "invalid when more than 11 digits",
"property": "clean",
"phrase": "321234567890",
"expected": null
}, I'll start a PR for the tests I mentioned in the earlier comment |
The problem brief mentions:
But the tests do not check for this requirement.
The text was updated successfully, but these errors were encountered: