mlavin / django-selectable

Tools and widgets for using/creating auto-complete selection widgets using Django and jQuery UI.
http://django-selectable.readthedocs.io/en/latest/
BSD 2-Clause "Simplified" License
129 stars 64 forks source link

Django 1.11 fixes #189

Closed spookylukey closed 7 years ago

spookylukey commented 7 years ago

This fixes the main issues described in #187

There are a few other tiny fixes/improvements to the tests that I hit along the way (in separate commits), but mainly I've tried to do it without rewriting the tests. The tests for the widget attributes did need changing significantly i.e. they now render the widgets, rather than call build_attrs on them, for reasons described in the commit messages. I tried to write the tests in ways that are future proof (i.e. don't assume that each widget will render a single HTML element), which is where some of the complexity comes from e.g. the AttrMap class.

codecov-io commented 7 years ago

Codecov Report

Merging #189 into master will increase coverage by 1.3%. The diff coverage is 95%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #189     +/-   ##
========================================
+ Coverage   93.89%   95.2%   +1.3%     
========================================
  Files          14      14             
  Lines         459     501     +42     
========================================
+ Hits          431     477     +46     
+ Misses         28      24      -4
Impacted Files Coverage Δ
selectable/tests/base.py 93.63% <88.23%> (ø)
selectable/forms/widgets.py 97.8% <98.48%> (+3.51%) :arrow_up:
selectable/views.py
selectable/__init__.py
selectable/base.py
selectable/models.py
selectable/tests/views.py 100% <0%> (ø)
example/core/models.py 100% <0%> (ø)
selectable/templatetags/__init__.py 100% <0%> (ø)

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 9c72e02...73b3800. Read the comment docs.

spookylukey commented 7 years ago

I've now fixed the remaining issues with this, added and fixed some tests, and fixed another warning in the test suite. I've also tested AutoCompleteSelectMultipleField in a real project running Django 1.11, which found some of the additional issues, but they are now fixed.

mlavin commented 7 years ago

This is :sparkles: amazing :sparkles: ! This compatibility shim is pretty complex but it's just a transition and can be dropped once < 1.11 support is eventually dropped. I don't really have any other feedback at the moment but I'm going to need a little more time to digest these changes and familiarize myself more with the 1.11 APIs. Thank you :heart: :heart: :heart:

spookylukey commented 7 years ago

@mlavin agreed that it is complex, more complex than I had hoped, but I couldn't find an easier way.

For the first main fix, the complexities came from: 1) the fact that we need to both provide build_attrs correctly so that it can be called from render in different Django versions, 2) the need to call build_attrs ourselves in our own render methods.

Additionally, there was: 3) the fact that for MultiWidget in Django 1.11, the render method of the subwidgets doesn't get called, so we need get_context instead. 4) test coverage didn't pick up item 3, I had to add tests for 'title' 5) some of the widget tests weren't plumbed the way that the fields work in practice, so those also had to be fixed.

Sorry for the complexity, as you say some of it will disappear in the future!

mlavin commented 7 years ago

Had a chance to more thoroughly review this and I think this is great. Thank you again for all of your work @spookylukey