Update django-phonenumber-field dep and tox deps

[craigloftus]

Bumping the requirements to django-phonenumber-field<2.99 and updating the deps in tox. I removed phonenumbers from the tox deps as that is no longer a direct dependency.

I've run this with tox locally with the exception of the py34 environment.

[codecov]

Codecov Report

Merging #272 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #272   +/-   ##
=======================================
  Coverage   96.52%   96.52%           
=======================================
  Files          39       39           
  Lines        1640     1640           
  Branches      116      116           
=======================================
  Hits         1583     1583           
  Misses         35       35           
  Partials       22       22

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ec4396...0c1d703. Read the comment docs.

[moggers87]

Shouldn't this match what's in setup.py?

[craigloftus]

Apologies, I had overlooked the lower bounds.

[Bouke]

phonenumbers is a direct dependency, see also: https://github.com/Bouke/django-two-factor-auth/blob/0ec439604abebfd087f3bb282ec8cc2333777b29/two_factor/templatetags/two_factor.py#L3

[craigloftus]

Ah. I have updated setup.py to include the dep then, rather than dropping it from tox.ini.

[Bouke]

We install the phonenumbers package in tox.ini and not in setup.py, because you can either install phonenumbers or phonenumberslite. The latter being a smaller database than the former.

The phonenumberslite version of the package does not include the geocoding, carrier and timezone metadata, which can be useful if you have problems installing the main phonenumbers package due to space/memory limitations.

When we add a hard dependency on phonenumbers, users can no longer opt to choose the lite dataset. See also #213.

However... once this commit gets released, that package always hard depends on phonenumbers, so this no longer holds true. Maybe we should do something similar, what do you think?

[craigloftus]

The question is somewhat moot, as once django-phonenumber-field is updated then users of django-two-factor-auth will get the full phonenumbers package anyway.

However... once this commit gets released, that package always hard depends on phonenumbers, so this no longer holds true. Maybe we should do something similar, what do you think?

I suggested a direct dependency on phonenumbers in that case (and this one) because it was the simple choice, and the 'cost' doesn't seem all that bad.

The memory footprint should be the same, the on disk size is 15M vs 50M, and download is 400K vs 3M (for wheels).

I think the long term solution is to get phonenumbers to use environment markers to allow the extras to be installed at the project level when needed.

[ryancausey]

It looks like in this PR, which appears released in version 2.1.0, django-phonenumber-field moves the dependency to extras_require and requires the developer to choose to install one or the other. Maybe django-two-factor-auth can take a similar approach?

For what it's worth, I've been using phonenumberslite and it doesn't appear to me that the usage in templatetags/two_factor.py specifically requires one or the other. I could be wrong, however.

[moggers87]

@ryancausey that's probably the best route to go (other than fixing phonenumbers' awful packaging)

We'd need a documentation update as well as updating the version of django-phonenumber-field that we require.

[ryancausey]

Not sure if this was the right way to do it, but @craigloftus I created a PR against your fork that should do the trick. I also merged master into it since github was complaining that the branch was out of date. Let me know if you have feedback.

[craigloftus]

@ryancausey I'd suggest you make a fresh PR and just reference this one in the description.