Skip to content
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: update to match canonical-data.json #106

Merged
merged 1 commit into from
Mar 14, 2017
Merged

phone-number: update to match canonical-data.json #106

merged 1 commit into from
Mar 14, 2017

Conversation

rpottsoh
Copy link
Member

Canonical-data.json recently changed. Updating test runner and example solution to align with new json.

@rpottsoh rpottsoh self-assigned this Mar 13, 2017
@rpottsoh rpottsoh requested a review from kotp March 13, 2017 14:55
@kotp
Copy link
Member

kotp commented Mar 13, 2017

This may need to be mentioned in x-common repository, but the tests for invalid_when_11 and valid_when_11 seem contradictory, if only due to lack of message in the test. Is there a way to have a custom message reported giving the reason why 11 is invalid, sometimes?

@rpottsoh
Copy link
Member Author

rpottsoh commented Mar 14, 2017 via email

@rpottsoh
Copy link
Member Author

@kotp instead of "invalid when 11 digits" how about "invalid when not starting with 1 and is 11 digits"? This would then seem to be balanced by "valid when 11 digits and starting with 1". If this seems reasonable I'll open an issue, or should I just submit a PR for the change?

@kotp
Copy link
Member

kotp commented Mar 14, 2017

You can make the change here, and potentially create that issue (or even a PR with the change) on x-common.

@kotp
Copy link
Member

kotp commented Mar 14, 2017

If you only make the change here, then when we do the generator we would still have the same issue potentially.

@rpottsoh
Copy link
Member Author

rpottsoh commented Mar 14, 2017

@kotp "invalid when 11 digits" is now "invalid when 11 digits not starting with 1". Slightly different wording from earlier. OK? If you're OK with it I'll open a PR in x-common to propose a change to the canonical-data.json. Would I need to bump the version, to what?

@kotp
Copy link
Member

kotp commented Mar 14, 2017

"invalid when 11 digits does not start with a 1"

@rpottsoh
Copy link
Member Author

I'll update this PR after exercism/problem-specifications#724 in x-common shakes out.

@rpottsoh
Copy link
Member Author

rpottsoh commented Mar 14, 2017

@kotp, canonical-data.json has been updated. I have updated my 2nd commit to reflect the change to the json description... Should the two commits be squashed or merged as is? @rbasso was all over my PR and approved the changes, a short lived PR.

@kotp
Copy link
Member

kotp commented Mar 14, 2017

Yes, no point in bringing in the change if it isn't up to date.

Canonical-data.json recently changed.  Updating test runner and example solution to align with new json.
@rpottsoh rpottsoh merged commit 87b54ea into exercism:master Mar 14, 2017
@rpottsoh rpottsoh deleted the phone-number branch March 14, 2017 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants