Skip to content

phone-number: Rewrite description and add tests #745

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

Merged
merged 2 commits into from
Apr 28, 2017
Merged

Conversation

behrtam
Copy link
Contributor

@behrtam behrtam commented Apr 4, 2017

Removes the part about how to test, as we have other docs for that. Now it also describes the telephone number system in a bit more detail.

Changes some test that used an invalid number but assumed it was valid.

Adds two tests that check the range of 2-9 for the first digit of the area and the exchange code.

Adds a test with a valid telephone number with country code and punctuation.

Removes the part about how to test, as we have other docs for that.
Now it also describes the telephone number system in a bit more detail.

Changes some test that used an invalid number but assumed it was valid.

Adds two tests that check the range of 2-9 for the first digit of the area
and the exchange code.

Adds a test with a valid telephone number with country code and punctuation.
@behrtam
Copy link
Contributor Author

behrtam commented Apr 4, 2017

We could also add new properties, but I think that's maybe to much.

{
  "description": "extract area code",
  "property": "area",
  "phrase": "(223) 456-7890",
  "expected": "223"
},
{
  "description": "extract exchange code",
  "property": "exchange",
  "phrase": "223.456.7890",
  "expected": "456"
},

Fits nice with those tracks that solve this with a class and use getter for the different number parts.

@behrtam behrtam changed the title number: Rewrite description and add tests phone-number: Rewrite description and add tests Apr 4, 2017
@kotp
Copy link
Member

kotp commented Apr 5, 2017

I like the tests with the leading number avoiding the 1. As far as the parts of the number, the names of those parts sometimes comes up in reviews (at least on the Ruby track).

```
where N is any digit from 2 through 9 and X is any digit from 0 through 9.

You task is to clean up differently formated telephone numbers by removeing punctuatin and the country code (1) if present.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be Your on line 12.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and "Punctuation" is misspelled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and "Removing"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx!
Sorry, for the sloppy work. I'll try to remember to use a spell checker next time.

@rpottsoh
Copy link
Member

@behrtam LGTM!

@behrtam
Copy link
Contributor Author

behrtam commented Apr 25, 2017

So, should I add the additional properties like area code or exchange code as proposed in the second comment or do we stick with the one existing property?

@rpottsoh
Copy link
Member

@behrtam I am fine with adding those tests. However, this PR back in December, touched on the issue of extraction and pretty printing and the decision was made to leave those tests out of the canonical-data.json.

@behrtam
Copy link
Contributor Author

behrtam commented Apr 25, 2017

@petertseng wrote:

Pretty printing and extracting subparts of numbers: Yeah, doesn't seem that important to me, and the README doesn't mention it, so I would not add them to the JSON file. It would make sense if people agreed to add it and it got added to the README and JSON file together. But that can come later.

Well those two additional properties are mentioned in the new description.

@rpottsoh
Copy link
Member

rpottsoh commented Apr 25, 2017 via email

@Insti
Copy link
Contributor

Insti commented Apr 25, 2017

those two additional properties are mentioned in the new description.

The conclusion in the previous PR was to not add them anyway.

I vote no, to adding the new properties.

@behrtam
Copy link
Contributor Author

behrtam commented Apr 25, 2017

I wouldn't really call a single post without interaction "conclusion" ... as I don't have any opinion about the additionl properties I'm fine with not adding them.

I will fix the typos later when I'm back at a real device.

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.

4 participants