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

Refactor backend filterset instantiation #865

Merged
merged 2 commits into from
Jul 13, 2018

Conversation

rpkilby
Copy link
Collaborator

@rpkilby rpkilby commented Jan 26, 2018

Possible compromise for #857. Right now, overriding the filterset is actually pretty inconvenient, because there is no hook into its instantiation aside from getting the class. To override the instance, it is necessary to completely overwrite both filter_queryset and to_html.

@rpkilby
Copy link
Collaborator Author

rpkilby commented Jan 26, 2018

Side note - @carltongibson I've noticed for a while that the CI builds seem to run jobs one at a time, instead of the usual five concurrently. Have current jobs been disabled/limited in the settings?

@codecov-io
Copy link

codecov-io commented Jan 26, 2018

Codecov Report

Merging #865 into master will decrease coverage by 0.54%.
The diff coverage is 89.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #865      +/-   ##
==========================================
- Coverage   98.41%   97.87%   -0.55%     
==========================================
  Files          15       15              
  Lines        1198     1128      -70     
==========================================
- Hits         1179     1104      -75     
- Misses         19       24       +5
Impacted Files Coverage Δ
django_filters/rest_framework/backends.py 93.75% <89.47%> (-1.84%) ⬇️
django_filters/utils.py 99.02% <0%> (-0.98%) ⬇️
django_filters/filters.py 98.42% <0%> (-0.97%) ⬇️
django_filters/fields.py 100% <0%> (ø) ⬆️
django_filters/views.py 100% <0%> (ø) ⬆️
django_filters/filterset.py 100% <0%> (ø) ⬆️
django_filters/rest_framework/filterset.py 100% <0%> (ø) ⬆️
django_filters/rest_framework/filters.py 100% <0%> (ø) ⬆️

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 04b087d...009b30f. Read the comment docs.

Copy link
Owner

@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.

Yea. OK. I could see subclassing the backend quickly next to the view.

Could you add notes to the docs?

@carltongibson carltongibson added this to the Version 2.0 milestone Jan 26, 2018
@rpkilby
Copy link
Collaborator Author

rpkilby commented Jan 26, 2018

Could you add notes to the docs?

Do you mean for the new methods? get_filter_class isn't currently documented.
Or do you mean a migration note for the get_filter_class => get_filterset_class rename?

@carltongibson
Copy link
Owner

I meant for the new methods. (But yes...)

(Build should be fixed BTW. I must have been on the mezcal.)

@rpkilby
Copy link
Collaborator Author

rpkilby commented Jan 26, 2018

Should View.filter_class and View.filter_fields be updated to .filterset_class and .filterset_fields?

This might be a good time to unify the DRF and Django view kwargs. The terminology is a little inconsistent, going between filter_* and filterset_*

@carltongibson
Copy link
Owner

Should ... be updated ...

It's probably a good idea yes. And 2.0 is the time for that. Can we raise a warning (to be removed in 2.1)?

@rpkilby
Copy link
Collaborator Author

rpkilby commented May 21, 2018

Note: After #867 is merged, this need to be rebased to account for the method renames/deprecations.

@lukyoung
Copy link

lukyoung commented Sep 30, 2020

Why didn't you pass the view argument into the returning data of the method django_filters.rest_framework.backends.DjangoFilterBackend.get_filterset_kwargs ?

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.

4 participants