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

Can't dissociate a possible phone number from a valid phone number #30

Closed
nrako opened this issue Mar 7, 2016 · 8 comments
Closed

Can't dissociate a possible phone number from a valid phone number #30

nrako opened this issue Mar 7, 2016 · 8 comments

Comments

@nrako
Copy link
Contributor

@nrako nrako commented Mar 7, 2016

libPhoneNumber has a way to separate a possible phone isPossibleNumber number from a valid one isValidNumber:

screen shot 2016-03-07 at 12 46 38

With PhoneNumberKit, this test fails:

    // Invalid brazilian number
    func testPossibleButInvalidNumber() {
        do {
            let phoneNumber = try PhoneNumber(rawNumber: "0765550055", region: "BR")
            phoneNumber.toE164()
            XCTFail()
        }
        catch {
            XCTAssert(true)
        }
    }

In PhoneNumberKit a parsed number is a "possible" number correct? Is it possible to dissociate a parsed (possible) number from a valid one? Since this use the same metadata than libPhoneNumber?

@marmelroy

This comment has been minimized.

Copy link
Owner

@marmelroy marmelroy commented Mar 7, 2016

Hi @nrako,

In PhoneNumberKit, parsing and validating were merged into the same action. A PhoneNumber object is always a valid number.

What exactly are you trying to achieve though? If you want to format an invalid number, the PartialFormatter could help you...

@nrako

This comment has been minimized.

Copy link
Contributor Author

@nrako nrako commented Mar 7, 2016

Hi @marmelroy, okay but as per my screenshot from the test with libPhoneNumber I would expect the number 0765550055 with region BR to fail and not be parsed.

@marmelroy

This comment has been minimized.

Copy link
Owner

@marmelroy marmelroy commented Mar 7, 2016

Ah okay. In that case there is indeed an issue here. What I suspect is that Google look at type UNKNOWN as invalid, whereas PhoneNumberKit doesn't. Will investigate further tonight...

@nrako

This comment has been minimized.

@nrako

This comment has been minimized.

Copy link
Contributor Author

@nrako nrako commented Mar 7, 2016

I was thinking of sending a PR, but I'm not sure that all PhoneNumber should be a valid number or valid number for a region.

I can see cases where I want the user to only be able to enter a valid phone number; e.g to receive a sms for validation, and optionally for a region specifically selected by the user. But there's cases where I just want to prevent dialling impossible/non-parsable number, but I'm fine to let the user try (and maybe fail) with an invalid number.

Moreover for performance reason it's probably best to just parseMultiple numbers without trying to validate them.

Maybe PhoneNumber should be considered as a "possible" number and have the isValidNumber and isValidNumber:forRegion methods? What do you think?

@marmelroy

This comment has been minimized.

Copy link
Owner

@marmelroy marmelroy commented Mar 7, 2016

Hmm... in terms of validations, PhoneNumberKit's number parsing does everything libPhoneNumber does in isValidNumber except for checking whether type != .Unknown. The type test can be very expensive and we already validate against the main country format (for the type test, you basically regex match against every available number type).

I'm leaning towards the Maybe PhoneNumber should be considered as a "possible" number and have the isValidNumber and isValidNumber:forRegion methods approach but want to think about this further.

@nrako

This comment has been minimized.

Copy link
Contributor Author

@nrako nrako commented Mar 7, 2016

@marmelroy 👍 let me know

@marmelroy

This comment has been minimized.

Copy link
Owner

@marmelroy marmelroy commented Apr 29, 2016

I added an isValidNumber computer property on the PhoneNumber object. I'm still not 100% sure it is the right approach though...

@marmelroy marmelroy closed this Apr 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.