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

Re-enable support for Django 1.3. #60

Merged
merged 8 commits into from
Oct 20, 2012

Conversation

benkonrath
Copy link
Contributor

I'm working on a feature for Django Rest Framework 2 that uses django-filter:

encode/django-rest-framework#290

Since Django Rest Framework 2 supports Django 1.3, I'd like to re-enable support for Django 1.3 in django-filter. We'd only while Django 1.3 is still supported (i.e. until Django 1.5 is released). What do you think?

@tomchristie
Copy link
Contributor

Planning add 1.3 to travis config in my fork to test.
Changeset seems small so if it all passes I'd obviously be a big +1 on this.

@benkonrath
Copy link
Contributor Author

I poked around a bit more and the changes to get the django-filter tests to pass are a little bigger than just that fist commit. Even with the latest changes, I'm still getting these errors on Django 1.3.3 with Python 2.7:

Creating test database for alias 'default'...
...............EE......E............
======================================================================
ERROR: test_char_filter (django_filters.tests.tests.FilterSetTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ben/Development/onepercentclub/django-filter/django_filters/tests/tests.py", line 266, in test_char_filter
    self.assertQuerysetEqual(f.qs, users, lambda o: o.pk, False)
TypeError: assertQuerysetEqual() takes at most 4 arguments (5 given)

======================================================================
ERROR: test_choice_filter (django_filters.tests.tests.FilterSetTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ben/Development/onepercentclub/django-filter/django_filters/tests/tests.py", line 421, in test_choice_filter
    self.assertQuerysetEqual(f.qs, ['aaron', 'alex', 'jacob'], lambda o: o.username, False)
TypeError: assertQuerysetEqual() takes at most 4 arguments (5 given)

======================================================================
ERROR: test_http_mapping (django_filters.tests.tests.FilterSetTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ben/Development/onepercentclub/django-filter/django_filters/tests/tests.py", line 442, in test_http_mapping
    self.assertQuerysetEqual(f.qs, ['jacob'], lambda o: o.username, False)
TypeError: assertQuerysetEqual() takes at most 4 arguments (5 given)

----------------------------------------------------------------------
Ran 36 tests in 0.782s

FAILED (errors=3)
Destroying test database for alias 'default'...

@tomchristie
Copy link
Contributor

Would be easy enough to fix: https://docs.djangoproject.com/en/dev/topics/testing/#django.test.TestCase.assertQuerysetEqual
https://github.com/django/django/blob/master/django/test/testcases.py#L746

One of:

  1. Provide an explicit order_by on the queryset and drop the last argument.
  2. Provide a base TestCase that implements .assertQuerySet for 1.3 compat (it's only a 3 line method)

@benkonrath
Copy link
Contributor Author

Ok, the tests pass for me in Django 1.3 and Django 1.4. @tomchristie Are you planning to send a pull request with your Travis configs? It would be great to see everything passing on the various versions.

@tomchristie
Copy link
Contributor

Great.

@benkonrath The travis configs are already pulled into django-filter master, just waiting to be turned on.

If you want to add Django 1.3 as part of this pull req and see them running against your fork...

  1. Make sure you've pulled the latest version of django-filter.
  2. Go to travis-ci.org, sign in, select your profile (top right), and turn on travis for your django-filter fork.
  3. Add django==1.3.3 to the .travis.yml config in this fork and push the change.

@apollo13 - What do you think to this pull req? Would def like to see 1.3 support and a PyPI version. Anything we could do to help?

@benkonrath
Copy link
Contributor Author

@tomchristie Thanks again for the help. Sorry that I didn't actually check master for that Travis stuff.

@apollo13
Copy link
Contributor

Hmm, I am wondering about one thing. Once 1.5 is released Django 1.3 won't get any security releases so it is somewhat odd to still support it in 3rd party apps.

@apollo13
Copy link
Contributor

You know what, let's just do a 1.3 release, a few nitpicks though, see the comments inline (coming in a minute)

@tomchristie
Copy link
Contributor

@apollo13 - Great, thanks! I think a lot of folks would still find 1.3 support useful right now. 1.4 was only released march this year after-all. Nice turnaround time from 1.4-1.5.

'%s__year' % name: now().year,
})),
}
if django.VERSION[:2] >= (1,4):
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this big if/else clause, all we use there is now() vs datetime.today() -- do that at the top of the file and just use now() in here

These changes are based on pull request feedback from Florian Apolloner.
Thanks!
@benkonrath
Copy link
Contributor Author

@apollo13 Thanks for the feedback. Definitely good suggestions as it makes things simpler in the future. Let me know if you have any other suggestions. Thanks again!

@apollo13
Copy link
Contributor

@benkonrath Is the extra fixture file needed due to the added timezone in the timestamps in 1.4 and friends?

@benkonrath
Copy link
Contributor Author

@apollo13 Yeah, the test_data_django_13 fixture is the same as the test_data fixture without the timezones on the timestamps.

apollo13 added a commit that referenced this pull request Oct 20, 2012
@apollo13 apollo13 merged commit 9954265 into carltongibson:master Oct 20, 2012
@benkonrath
Copy link
Contributor Author

@apollo13 Thanks for merging this in. Would it be possible to make a release for with this change on pypi? This will make it easier to add django-filter as a requirement to Django REST framework 2. Thanks again for your feedback on this pull request.

@apollo13
Copy link
Contributor

apollo13 commented Nov 1, 2012

Yes, I'll have to figure out how to do releases first and then coordinate with @alex to get PyPi acces ;)

@benkonrath
Copy link
Contributor Author

Great to hear! I'll keep my eyes on pypi for the release.

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