modlinltd / django-advanced-filters

Add advanced filtering abilities to Django admin
https://pypi.org/project/django-advanced-filters/
MIT License
771 stars 173 forks source link

fix for Django 2.0, update tests #63

Closed PetrDlouhy closed 6 years ago

PetrDlouhy commented 6 years ago

Fixes for Django 2.0

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.04%) to 96.104% when pulling 8837bca3a319057351daf7a2c6ef101a7fd98e0d on PetrDlouhy:develop into b243e0688e3d9f2fd09a4d70b9237611aaac5df5 on modlinltd:develop.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 96.06% when pulling 956a02b55e0c8c216cf0b31525decf43e1086e19 on PetrDlouhy:develop into 1a482cf85709d28adefe6b4444eac8043eed701b on modlinltd:develop.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 96.06% when pulling 956a02b55e0c8c216cf0b31525decf43e1086e19 on PetrDlouhy:develop into 1a482cf85709d28adefe6b4444eac8043eed701b on modlinltd:develop.

asfaltboy commented 6 years ago

Thanks @PetrDlouhy that's mighty kind of you! I think Travis must use some cached mirror doesn't have Django 2.0.1 on it yet.

PetrDlouhy commented 6 years ago

@asfaltboy No, Django 2.0 doesn't support Python 2, that is cause for the fail of the first two builds. The cause of the Python 3 build is that there will be needed more compatibility changes.

I will resolve those problems.

PetrDlouhy commented 6 years ago

@asfaltboy I fixed all the tests. The failing tests are due to django-braces which are not yet released for Django 2.0.

cyberbudy commented 6 years ago

Any progress on it?

asfaltboy commented 6 years ago

Sorry @cyberbudy , we're sort of blocked by 3rd party package compatibility issue (brack3t/django-braces#233).

You may still use PetrDlouhy's develop branch try to workaround the issue by providing your own copy of the mixins instead of braces' ones (that won't fail in 2.0), as here:

https://github.com/modlinltd/django-advanced-filters/blob/1a482cf85709d28adefe6b4444eac8043eed701b/advanced_filters/views.py#L21-L22

lnxg33k commented 6 years ago

django-braces bumped the code to cover django 2 https://github.com/brack3t/django-braces/commit/8b2f4122ea02f907ad9e9e29fa19d90f594ecebb

asfaltboy commented 6 years ago

Thanks for the reminder @lnxg33k , I pushed a dependency commit to this PR, if all is well we should be able to merge it.

asfaltboy commented 6 years ago

Seems like some Q() object serialisation assertions are actually failing now on Python 3.5 w/ Django 2 in Travis. We'll need to have a look at this before we merge, help is appreciated!

peppelinux commented 6 years ago

url.py is not Django 2.0 compliant, it simply doesn't work

mrname commented 6 years ago

Might want to add a database migration in this PR. I just ran makemigrations from your branch, and got some updates:

Migrations for 'advanced_filters':
  /usr/local/lib/python3.6/site-packages/advanced_filters/migrations/0003_auto_20180606_1855.py
    - Alter field created_at on advancedfilter
    - Alter field created_by on advancedfilter
    - Alter field groups on advancedfilter
    - Alter field title on advancedfilter
    - Alter field url on advancedfilter
    - Alter field users on advancedfilter
peppelinux commented 6 years ago

This may help, actually I'm using this in Django 2.0: https://github.com/modlinltd/django-advanced-filters/pull/73

anuj9196 commented 6 years ago

Is there any update for Dajngo 2.0?

asfaltboy commented 6 years ago

I've corrected the last failing test and rebased over develop. @PetrDlouhy I would really ❤️ a cross-review for the latest changes if you can.

@anuj9196 @peppelinux @cyberbudy please feel free to try this branch out and let us know if you find anything peculiar.

mrsarm commented 6 years ago

When will this merge? I tested the branch proposed in my project with Django 2.0.7 and Python 3.6 and it worked!

For those that need this asap, add this to your requirements.txt:

-e git://github.com/PetrDlouhy/django-advanced-filters.git@e769781ac489dfd408c2f872b2d60603451fc677#egg=django-advanced-filters
PetrDlouhy commented 6 years ago

@asfaltboy I looked at the changes, and they look good to me. Thanks for the except fix, that was oversight from my side.

Edit: the following part was written before I saw the issue #79. So I will continue the discussion there.

There is one think, I thing is not right (but was not right before this change). And it is the install_requires in setup.py. I think, we shouldn't stick to one version. It could lead to unsolvable dependencies problem. I think, we should change it to:

    install_requires=[
        'django-braces>=1.4.0',
        'simplejson>=3.6.5',
    ],
asfaltboy commented 6 years ago

Thanks for the review Petr! ~Together with PR #70 (regarding missing migrations)~ This will make for a nice small "support release".