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

Add utility method to format phone numbers #928

Merged

Conversation

mkrausse-ggtx
Copy link
Contributor

This pull request adds a new utility method, format_phone_number, which formats a phone number in E.164 format. The method takes a phone number as input and an optional country code, and returns the formatted phone number. It removes non-numeric characters and leading zeros, adds the country code prefix if necessary, and formats the number in E.164 format. The pull request also includes unit tests for the format_phone_number method to ensure its correctness and handle different scenarios.

@austinweisgrau
Copy link
Collaborator

IMO a better default behavior for a format phone number method would be to return None for an invalid phone number rather than raise an error.

@mkrausse-ggtx
Copy link
Contributor Author

That makes sense to me. I can update that.

@shaunagm
Copy link
Collaborator

Another option would be to add some kind of error_on_invalid option which defaults to false, but allows people to proactively say "yes this absolutely needs a number, error if it doesn't exist". Users could always write code to check the output and error on none, but it might be worth making it more convenient for them.

@mkrausse-ggtx
Copy link
Contributor Author

I'm happy to do either one. Do folks think my approach to not adding another dependency makes sense? I'm also willing to write a function that uses the phonenumbers package.

@austinweisgrau
Copy link
Collaborator

Yeah I think your approach is good! The dependency seems unnecessary.

A few other validations worth incorporating potentially - valid US phones can't have an area code that starts with 0 or 1, and the first digit after the area code also can't be 0 or 1. I think 999 is also an invalid area code.

FWIW the phonenumbers package doesn't invalidate those patterns so there's no advantage there either.

@shaunagm
Copy link
Collaborator

It might be worth looking at the things phonenumbers does validate in case there's anything we want to copy. Although I took a quick look at the docs/code and it's not clear to me what their full list of checks are. Oh well! Regardless, I agree that it's overkill given Parsons users are primarily/mostly using US numbers.


def format_phone_number(phone_number, country_code="1"):
"""
Formats a phone number in E.164 format.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add some documentation showing what it'll do? That would be helpful to someone who doesn't know what the e.164 format is, and might have no idea what this does.

Even one line like:

Example: Converts "(123) 123-1212" to "+01231231212"

would be very helpful!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Went ahead and made that change.

@shaunagm shaunagm merged commit e631500 into move-coop:main Dec 19, 2023
6 checks passed
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.

5 participants