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

Fixed schema generation for filter backends #5613

Merged
merged 1 commit into from
Nov 22, 2017

Conversation

D3X
Copy link
Contributor

@D3X D3X commented Nov 20, 2017

Description

This contains a test for #5594 as well as a fix for that issue.

Fixes: #5594

@D3X D3X mentioned this pull request Nov 20, 2017
6 tasks
@@ -951,3 +953,36 @@ def custom_action(self, request, pk):

assert inspector.should_include_endpoint(path, callback)
assert inspector.get_allowed_methods(callback) == ["GET"]


@unittest.skipUnless(coreapi, 'coreapi is not installed')
Copy link
Member

Choose a reason for hiding this comment

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

The tests should probably use a builtin filter backend (eg, search or ordering), or skip the test if django_filters is not installed (especially since it's not a direct test dependency anymore).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. I sort of assumed it only breaks with DF, as described in the issue. I'll change that.

('foo', False),
('FOO', False),
]
)
Copy link
Member

Choose a reason for hiding this comment

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

idk why parameterize is failing on py2k. I'd consider changing this to a class with a few test methods instead of parameterization.

)
@modify_settings(INSTALLED_APPS={'append': 'django_filters'})
def test_auto_schema_allows_filters(method, expected):
class AFilter(django_filters.FilterSet):
Copy link
Member

@rpkilby rpkilby Nov 21, 2017

Choose a reason for hiding this comment

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

Minor nitpick: "A" and "An" prefixes don't help the understandability of the test objects. I'd probably just use Filter and APIView instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the pattern from that test file. There are classes like AViewSet and AnAPIView in the tests above.

@carltongibson carltongibson added this to the v3.7.4 milestone Nov 21, 2017
Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @D3X.

@rpkilby I let you drive this one. 👍

@D3X D3X force-pushed the 5594-fix-schema-generation branch 2 times, most recently from af5ec83 to d0d51a1 Compare November 21, 2017 10:45
Refactored the auto schema allows filters test.

Removed an unused import.

Removed another unused import.
@D3X D3X force-pushed the 5594-fix-schema-generation branch from d0d51a1 to 913ec16 Compare November 21, 2017 10:50
@D3X
Copy link
Contributor Author

D3X commented Nov 21, 2017

@rpkilby I've refactored the test. The build is green now :)

@rpkilby rpkilby changed the title #5594 Fixed schema generation. Fixed schema generation for filter backends Nov 22, 2017
@rpkilby rpkilby merged commit 134a6f6 into encode:master Nov 22, 2017
@rpkilby
Copy link
Member

rpkilby commented Nov 22, 2017

LGTM. Thanks again @D3X

@D3X D3X deleted the 5594-fix-schema-generation branch November 22, 2017 09:21
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
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.

Filter backend schema inspector issue
3 participants