Skip to content

Make GrapheneFilterSetMixin compatible with django-filter 2 #492

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

Merged
merged 5 commits into from
Sep 5, 2018
Merged

Make GrapheneFilterSetMixin compatible with django-filter 2 #492

merged 5 commits into from
Sep 5, 2018

Conversation

jayhale
Copy link

@jayhale jayhale commented Aug 7, 2018

To support Django >=2 graphene-django must support django-filter >=2.

Changes made:

  • Revised graphene_django.filter.GlobalIDFilter to handle cases when the filter value is None.
  • Removed filter_for_reverse_field() from graphene_django.filter.GrapheneFilterSetMixin
  • Added back filter_for_reverse_field() to graphene_django.filter.GrapheneFilterSetMixin only for cases when django-filter <2
  • Updated requirements in setup.py to Django >=1.11 and django-filter >=2 for python ==3 and django-filter >=1 for python ==2
  • Removed unnecessary checks for Django <1.9 from graphene_django/tests/test_converter.py
  • Removed separate check for JSONField in compat.py
  • Updated requirements in examples/cookbook
  • Updated links to django-filter documentation in docs/filtering.rst

All tests pass.

Resolves: #484
Resolves: #464

Related: #491

@coveralls
Copy link

coveralls commented Aug 7, 2018

Coverage Status

Coverage decreased (-0.08%) to 94.583% when pulling 0314931 on jayhale:django-filter-2 into 14f156e on graphql-python:master.

@phalt
Copy link
Contributor

phalt commented Aug 13, 2018

👍 if this brings compatibility with django 2.1!

Copy link

@afriapps afriapps left a comment

Choose a reason for hiding this comment

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

This works, tested with Django 2 and django-filter 2

@synasius
Copy link

synasius commented Aug 16, 2018

@jayhale PR looks great! Just a couple of notes:

@patrys patrys mentioned this pull request Aug 16, 2018
6 tasks
@jayhale
Copy link
Author

jayhale commented Aug 16, 2018

@synasius thanks. I've:

  • Added back a test case for Python 2.7 and Django 1.11
  • Added back linting for Python 2.7 (rules appear to hit differently whether I'm on 2.7 or 3.x, so including both)
  • Removed unnecessary checks for Django < 1.9 from graphene_django/tests/test_converter.py
  • Removed separate check for JSONField in compat.py
  • Fixed a typo in a link I updated in the filtering docs (all correct now)

Hope that closes this one out. Cheers!


Tests passed: https://travis-ci.org/jayhale/graphene-django/builds/416952174 - just a stalled build in this workstream

@synasius
Copy link

@jayhale great! LGTM 👍

@synasius
Copy link

@jayhale Can you trigger the tests again, so that we a have a "green" PR ?

@jayhale
Copy link
Author

jayhale commented Aug 17, 2018

@sciyoshi I made a nominal change for the sake of re-running the tests.

Only those with write-access to the repo are able to restart builds, so it takes a new commit to restart. As an alternative to pushing a new commit, anyone with write access can always restart a build from the Travis build status page for the PR (follow the details link next to the task on GitHub).

Glad to be able to help with this one. Thanks for a helpful library.

Copy link

@eieste eieste left a comment

Choose a reason for hiding this comment

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

Works for me too 👍

@b99andla
Copy link

Waiting for a release on this one since its blocking our development! Thanks in advance!

@ValentinCreative
Copy link

@b99andla I was in the same situation.
I temporarily put this in my requirements.txt
git+https://github.com/jayhale/graphene-django.git@django-filter-2#egg=graphene-django
Instead of graphene-django
So I'm able to continue developments, if it can help :)

@aldarund
Copy link

This should go live, its blocking use of django 2.1

@aldarund
Copy link

aldarund commented Sep 1, 2018

@syrusakbary any plans on merging this anytime soon..?

@patrick91
Copy link
Member

@jayhale can you resolve the conflicts in docs/filtering.rst?

@jayhale
Copy link
Author

jayhale commented Sep 4, 2018

Merge conflict resolved.

@patrick91
Copy link
Member

Pinging @syrusakbary since I can't merge :)

@aldarund
Copy link

aldarund commented Sep 5, 2018

Would be good to also release a new version after merge :)

@syrusakbary
Copy link
Member

The PR looks great! Merging

@syrusakbary syrusakbary merged commit e45708b into graphql-python:master Sep 5, 2018
@jckw
Copy link
Contributor

jckw commented Sep 5, 2018

Nice! When can we expect a release to PyPI @syrusakbary ?

@syrusakbary
Copy link
Member

By EOD today or tomorrow :)

@syrusakbary
Copy link
Member

A new version of Graphene Django: 2.2.0 is now on PyPI.

Here are the release notes: https://github.com/graphql-python/graphene-django/releases/tag/v2.2.0

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.