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

django-phonenumber-field #33

Closed
jayvdb opened this issue Apr 24, 2020 · 4 comments
Closed

django-phonenumber-field #33

jayvdb opened this issue Apr 24, 2020 · 4 comments
Labels
low priority issues that would be nice to fix but have low impact

Comments

@jayvdb
Copy link
Contributor

jayvdb commented Apr 24, 2020

https://pypi.org/project/django-phonenumber-field/ 2.0.1 (unfortunately locked to that by current django-oscar & django-oscar-api & django-oscar-api-checkout):

could not resolve serializer field ModelField(help_text='In case we need to call you about your order', model_field=<phonenumber_field.modelfields.PhoneNumberField: phone_number>, required=False, validators=[<function validate_international_phonenumber>, <django.core.validators.MaxLengthValidator object>]). defaulting to "string"

The master versions should let me update to django-phonenumber-field v3, but poetry is making that difficult to upgrade with git (python-poetry/poetry#2336). Doing a fresh install to get around that.

@tfranzel
Copy link
Owner

not quite sure what to do here.

can't find ModelField in https://github.com/stefanfoulis/django-phonenumber-field.

not sure, but i believe https://github.com/stefanfoulis/django-phonenumber-field/blob/master/phonenumber_field/serializerfields.py#L8 should work because its is based on a CharField.

@tfranzel tfranzel added the question Further information is requested label Apr 24, 2020
@jayvdb
Copy link
Contributor Author

jayvdb commented Apr 24, 2020

The ModelField invocation is in https://github.com/django-oscar/django-oscar/blob/a22f4991b036890b2d90839cd2c4a03e22258255/src/oscar/apps/address/abstract_models.py

I've managed to do a build with oscar "master", which uses django-phonenumber-field v3, and the warning has disappeared, so I suspect this only occurs in django-phonenumber-field v2.

However, the datatype when using django-phonenumber-field v3 remained as "string", which is a sane and possibly even correct datatype for this field, so this is almost a non-issue except for the annoyance. I do not intend to be using oscar "master" in production, so until they do a release I guess I live with the warning or add a helper to mask the warning. Feel free to close if django-phonenumber-field v2 isnt worth supporting.

@tfranzel
Copy link
Owner

i understand. i don't plan on introducing hacks for specific libraries, but maybe this gets solved through some other generalization or introspection improvements. i'll leave it open for now but make it low prio. please close if you notice that the issue disappeared. thanks!

@tfranzel tfranzel added low priority issues that would be nice to fix but have low impact and removed question Further information is requested labels Apr 24, 2020
@tfranzel
Copy link
Owner

tfranzel commented May 7, 2020

problem was that the old model class was class PhoneNumberField(models.Field) and the new class is class PhoneNumberField(models.CharField).

stefanfoulis/django-phonenumber-field@fe946db#diff-497bc1cf2ee9e2f3444d450019a283c4L43

code is now more graceful for the old variant by trying to use get_internal_type before bailing.

@tfranzel tfranzel closed this as completed May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low priority issues that would be nice to fix but have low impact
Projects
None yet
Development

No branches or pull requests

2 participants