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

Remove dependency future #247

Merged
merged 3 commits into from
Jul 10, 2017
Merged

Remove dependency future #247

merged 3 commits into from
Jul 10, 2017

Conversation

benjaoming
Copy link
Contributor

@benjaoming benjaoming commented Jul 9, 2017

The use of future is causing problems because we are bundling dependencies statically for a distribution that targets py2+py3.

It was introduced because of #4 in PR #5

Hard to believe that someone would bundle dependencies, but in our case it's very relevant because we distribute python packages for net-less installation with everything bundled in. We can't use django-q for our packages because future is raising exceptions when loaded on Python 3:

learningequality/kolibri#1797

Anyways, it wasn't used in more than 1 single instance of from builtins import range.

Here are some other advantages:

  • future redefines and monkey patches sys.modules for all other packages, perhaps not nice!
  • It has 106 unsolved issues
  • Removes a dependency, so makes everything nicer :)
  • future was missing in README.rst description

I've fixed some more cases where range wasn't loaded in a backwards compatible way. You can say that even though future was added as a dependency, it wasn't used in a couple of cases where it should have (test code).

Reference

There was only one case of accessing something redefined by futures library.

ack-grep --python 'import.*queue'
ack-grep --python 'reprlib'
ack-grep --python 'socketserver'
ack-grep --python 'winreg'
ack-grep --python 'test.support'
ack-grep --python 'import.*html'
ack-grep --python 'html.parser'
ack-grep --python 'html.entites'
ack-grep --python 'import.*http'
ack-grep --python 'http.client'
ack-grep --python 'http.server'
ack-grep --python 'http.cookies'
ack-grep --python 'http.cookiejar'
ack-grep --python 'urllib.parse'
ack-grep --python 'urllib.request'
ack-grep --python 'urllib.response'
ack-grep --python 'urllib.error'
ack-grep --python 'urllib.robotparser'
ack-grep --python 'xmlrpc.client'
ack-grep --python 'xmlrpc.server'
ack-grep --python '_thread'
ack-grep --python '_dummy_thread'
ack-grep --python '_markupbase'
ack-grep --python 'itertools.*filterfalse'
ack-grep --python 'itertools.*zip_longest'
ack-grep --python 'sys.*intern'
ack-grep --python 'collections.*UserDict'
ack-grep --python 'collections.*UserList'
ack-grep --python 'collections.*UserString'
ack-grep --python 'collections.*OrderedDict'
ack-grep --python 'collections.*Counter'
ack-grep --python 'subprocess.*'
ack-grep --python 'subprocess.*getoutput'
ack-grep --python 'subprocess.*getstatusoutput'
ack-grep --python 'subprocess.*check_output'

cluster.py
7:from builtins import range 

@benjaoming benjaoming changed the title Remove unused dependency futures Remove unused dependency future Jul 9, 2017
@benjaoming benjaoming changed the title Remove unused dependency future Remove unused dependency future Jul 9, 2017
@codecov-io
Copy link

codecov-io commented Jul 9, 2017

Codecov Report

Merging #247 into master will decrease coverage by 2.47%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #247      +/-   ##
==========================================
- Coverage   93.19%   90.71%   -2.48%     
==========================================
  Files          40       41       +1     
  Lines        2717     2724       +7     
==========================================
- Hits         2532     2471      -61     
- Misses        185      253      +68
Impacted Files Coverage Δ
django_q/tests/test_brokers.py 70.58% <100%> (-11.7%) ⬇️
django_q/compat.py 100% <100%> (ø)
django_q/tests/test_monitor.py 100% <100%> (ø) ⬆️
django_q/tests/test_cluster.py 100% <100%> (ø) ⬆️
django_q/tests/test_cached.py 100% <100%> (ø) ⬆️
django_q/cluster.py 91.34% <100%> (-0.05%) ⬇️
django_q/brokers/ironmq.py 0% <0%> (-94.74%) ⬇️
django_q/brokers/__init__.py 94.18% <0%> (-2.33%) ⬇️

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 2190d63...5978165. Read the comment docs.

@Koed00
Copy link
Owner

Koed00 commented Jul 9, 2017

Nice. The 2.7 compatibility was a bit of an afterthought and I haven't had time to improve it.
At this point I will probably remove 2.7 when Django 2.0 arrives. Thanks for the effort; I will merge it soon.

@benjaoming
Copy link
Contributor Author

Django 2.0 will also remove Python 2.7 compatibility - 1.11 is the last to support it: https://docs.djangoproject.com/en/1.11/releases/1.11/#python-compatibility

We have a couple of more additions in our internal branch that I'll ask people to PR upstream - thanks for the great project @Koed00 :)

@Koed00 Koed00 merged commit d33752f into Koed00:master Jul 10, 2017
@benjaoming benjaoming changed the title Remove unused dependency future Remove dependency future Jul 10, 2017
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.

3 participants