stefanfoulis / django-phonenumber-field

A django model and form field for normalised phone numbers using python-phonenumbers
MIT License
1.48k stars 318 forks source link

Add get_str_value and split_representation Support In Phone Number Serializer Field #579

Closed keshavmohta09 closed 11 months ago

keshavmohta09 commented 11 months ago

Add get_str_value and split_representation support

francoisfreitag commented 11 months ago

Hi, thanks for submitting a PR!

Could you explain why these changes should be made?

Currently, I am thinking of rejecting both:

keshavmohta09 commented 11 months ago

Hello, thanks for viewing the changes.

I agree that the phone number instance should be returned from the to_internal_value method, but in many cases, we need its string value, so I have added a flag.

In the case of split_representation, in most cases the country code and phone number need to be sent separately to the frontend, and displayed separately, that's why I have added and we can also handle accepting the same format if needed.

francoisfreitag commented 11 months ago

I agree that the phone number instance should be returned from the to_internal_value method, but in many cases, we need its string value, so I have added a flag.

Why not call str() on the object returned from the serializer ?

In the case of split_representation, in most cases the country code and phone number need to be sent separately to the frontend, and displayed separately, that's why I have added and we can also handle accepting the same format if needed.

It really depends on the use case. Some projects will just use an <input type="tel">, others will have a separated <select> for the country code, and only ask for the national significant parts (see the existing widgets for alternatives). There are unofficial JS implementations for Google libphonenumbers, which allow to manipulate the phone number in the front-end, so developers can be creative. As far as this library is concerned, sending a string phone number that follows one of the supported formats seems a reasonable default to me. Projects needing their own representation can easily override to_representation(). Barring a compelling argument, I think we should stick to the current implementation.

keshavmohta09 commented 11 months ago

Okay, thankyou for guiding me. I am closing this PR.