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

Upgrade phone number field #178

Closed
wants to merge 3 commits into from
Closed

Upgrade phone number field #178

wants to merge 3 commits into from

Conversation

emord
Copy link
Contributor

@emord emord commented Nov 8, 2016

@Bouke

@@ -65,7 +65,7 @@
'two_factor',
'example',

'debug_toolbar',
# 'debug_toolbar',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you try uncommenting the middleware to see if it now works under Django 1.10?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't. runserver ends with an error NoReverseMatch: u'djdt' is not a registered namespace

Seems like making it work should be part of another PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

With version 1.6 of Django Debug Toolbar that should be resolved, can you verify that you're on that version?

Anyway I think its best to separate it from this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was on 1.6, the issue was that the app didn't include the djdt urls. I've fixed that in #179

Copy link
Collaborator

@Bouke Bouke left a comment

Choose a reason for hiding this comment

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

Overall LGTM. However one issue that might arise is that this might not work with wheel packages. As the setup.py is not run on install (as far as I know), this would mean that the phonenumberlite dependency would not be installed. See also wheel conditional dependencies.

'django-formtools',
]
if 'phonenumbers' not in [p[1] for p in pkgutil.iter_modules()]:
install_requires.append('phonenumberslite>=7.0.9')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you change this to ...,<7.99, assuming they semver, would prevent breaking changes.

'Django>=1.8',
'django_otp>=0.3.4,<0.99',
'qrcode>=4.0.0,<4.99',
'django-phonenumber-field>=1.1.0,<1.99.0',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you change this to ...<1.99 instead; so to be consistent with the other version notations?

@coveralls
Copy link

coveralls commented Nov 9, 2016

Coverage Status

Coverage remained the same at 95.956% when pulling 0b535a0 on emord:upgrade-phone-number-field into 543687e on Bouke:master.

Copy link
Collaborator

@MarkusH MarkusH left a comment

Choose a reason for hiding this comment

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

On the note what @Bouke already mentioned: Wheels don't run setup.py thus this won't work as expected.

@emord
Copy link
Contributor Author

emord commented Nov 10, 2016

@Bouke @MarkusH I looked into the conditional dependencies on wheel, but that didn't seem like the right approach as it wouldn't install either by default which could cause this package to break. Instead I changed the phonenumbers dependency to phonenumberslite. This way it will only install the necessary dependency if this is the only package that requires phonenumbers, but a user can also install the full package on top of it. Thoughts?

@coveralls
Copy link

coveralls commented Nov 10, 2016

Coverage Status

Coverage remained the same at 95.956% when pulling f1fab5f on emord:upgrade-phone-number-field into 7a4b21d on Bouke:master.

@coveralls
Copy link

coveralls commented Nov 10, 2016

Coverage Status

Coverage remained the same at 95.956% when pulling 7ff5a99 on emord:upgrade-phone-number-field into 7a4b21d on Bouke:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.956% when pulling 7ff5a99 on emord:upgrade-phone-number-field into 7a4b21d on Bouke:master.

@coveralls
Copy link

coveralls commented Nov 10, 2016

Coverage Status

Coverage remained the same at 95.956% when pulling 7ff5a99 on emord:upgrade-phone-number-field into 7a4b21d on Bouke:master.

@MarkusH
Copy link
Collaborator

MarkusH commented Nov 10, 2016

I'm trying to figure out the difference between phonenumbers and phonenumberslite. The latter seems to have the same size when you look at the .whl file. The .tar.gz source file is considerably smaller, though. But since pip prefers wheel packages these days I don't see a benefit in actually changing those dependencies ... . Can you give some background why you would like to have this changed?

@coveralls
Copy link

coveralls commented Nov 10, 2016

Coverage Status

Coverage remained the same at 95.956% when pulling 7ff5a99 on emord:upgrade-phone-number-field into 7a4b21d on Bouke:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.956% when pulling 7ff5a99 on emord:upgrade-phone-number-field into 7a4b21d on Bouke:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.956% when pulling 7ff5a99 on emord:upgrade-phone-number-field into 7a4b21d on Bouke:master.

@emord
Copy link
Contributor Author

emord commented Nov 10, 2016

@MarkusH https://github.com/daviddrysdale/python-phonenumbers#memory-usage

The last paragraph in that section describes the difference between packages. I'm not too familiar with wheel vs tar.gz packages to be honest. Before 7.6.0 there was only the tar.gz format which is supported here.

I proposed this change as phonenumberslite as we use it in our repository and ran into an issue with our deploy process when this installed phonenumbers (which we fixed). There wasn't a need for the full package and the field package was behind as well, so I figured it wouldn't hurt

@Bouke
Copy link
Collaborator

Bouke commented Feb 7, 2017

I've updated the dependency. As the default dependency library for django-phonenumber-field changed to phonenumberslite, there is no longer a need to replicate the dependency in this package. So I've taken it out.

@Bouke Bouke closed this Feb 7, 2017
@emord emord deleted the upgrade-phone-number-field branch February 9, 2017 14:45
@emord
Copy link
Contributor Author

emord commented Feb 9, 2017

@Bouke Thanks! Could you release a version with that update?

For what it's worth, I think it may have been dangerous (although unlikely to cause issues) to take out phonenumbers as a dependency. It's used explicitly here https://github.com/Bouke/django-two-factor-auth/blob/f308883d7e1e327f06080da254c75a20e5fcc4fb/two_factor/migrations/0003_auto_20150817_1733.py#L6 and some other places. I think if that is removed from a dependency in django-phonenumber-field this library could cause error

@emord emord restored the upgrade-phone-number-field branch February 9, 2017 14:50
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.

None yet

5 participants