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

Add nested related filters to docs schema #320

Closed
wants to merge 1 commit into from

Conversation

Ken4scholars
Copy link

Currently, nested filters are not shown in swagger docs because they are not added to the OpenAPI schema. With this PR, the filters are expanded and added when generating the docs

@codecov-io
Copy link

Codecov Report

Merging #320 into 0.11.x will decrease coverage by 5.26%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           0.11.x     #320      +/-   ##
==========================================
- Coverage   98.33%   93.06%   -5.27%     
==========================================
  Files           4        4              
  Lines         180      202      +22     
  Branches       41       46       +5     
==========================================
+ Hits          177      188      +11     
- Misses          1       11      +10     
- Partials        2        3       +1
Impacted Files Coverage Δ
rest_framework_filters/backends.py 67.74% <23.07%> (-32.26%) ⬇️
rest_framework_filters/filterset.py 98.41% <88.88%> (-0.74%) ⬇️

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 331b23e...4772d7d. Read the comment docs.

@Ken4scholars
Copy link
Author

Please review and let me know if I missed anything. This is my first time opening a PR here

Copy link
Collaborator

@rpkilby rpkilby left a comment

Choose a reason for hiding this comment

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

Hi @Ken4scholars. Apologies that I'm going to be a bit terse, but I can go into more detail if you have questions. I left a few comments below, but additionally:

  • Schema support should be tested.
  • Highly nested and recursive relationships should be considered (i.e., how deep do you go?). By default, the backend renders forms with a depth of 1. Look into disable_subset.

rest_framework_filters/filterset.py Show resolved Hide resolved
rest_framework_filters/filterset.py Show resolved Hide resolved
rest_framework_filters/filterset.py Show resolved Hide resolved
rest_framework_filters/backends.py Show resolved Hide resolved
@Ken4scholars
Copy link
Author

Hello @rpkilby. Thank you for the review and pointing out things that should be considered.

  • I will try to add tests for the schema generation
  • Will also look at the recursion depth. For now, it retrieves every filter down to the tree leaves. In cases like AllFilter that is sure going to bloat the docs so I will check how the forms are implemented and use a similar strategy.

@rpkilby
Copy link
Collaborator

rpkilby commented Nov 15, 2019

Hi @Ken4scholars. Given that the PR is targeting 0.11.x, I'm going to go ahead and close it. Happy to reconsider a PR targeting the master branch and django-filter 2.

@rpkilby rpkilby closed this Nov 15, 2019
@Ken4scholars
Copy link
Author

Ken4scholars commented Nov 15, 2019

Hello @rpkilby sorry, but any reason why there shouldn't be new PRs for 0.11x? 1.00 is still in dev mode and has not been released yet. Is it a project policy? Just to be clear, I considered this a fix and not a new feature and that's why I decided to push it to the latest branch in use instead of waiting for the next big 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.

None yet

3 participants