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 filter_fields to FilterView #632

Merged
merged 2 commits into from Feb 14, 2017

Conversation

@marctc
Copy link

@marctc marctc commented Feb 14, 2017

If you have a FilterView only providng a model it could be useful to specify the fields you want to include to the filterset automatic construction.

If you have a FilterView only providng a model it could be
useful to specify the fields you want to include or exclude to
filterset automatic construction.
@carltongibson
Copy link
Owner

@carltongibson carltongibson commented Feb 14, 2017

Hi @marctc.

This seems a reasonable addition.

I'm kind of of the opinion that ever adding exclude to whatever API it first appeared in was an error. Setting fields is sometimes more effort but often a maintenance win in the long run. i.e. I wonder if we really want it here. What's your thought on that?

@marctc
Copy link
Author

@marctc marctc commented Feb 14, 2017

I'm kind of of the opinion that ever adding exclude to whatever API it first appeared in was an error.

I added because sometimes could be useful to exclude some fields and also to emulate the behaviour of Meta classes (like FieldSets, Serializers or Forms). But OTOH neither Django generic views nor DRF views had a an exclude fields parameter.

Should I remove then?

@carltongibson
Copy link
Owner

@carltongibson carltongibson commented Feb 14, 2017

Yeah. I think so.

Also, it should probably be filter_fields to match DRF.

Did you need this yourself? (The minimal FilterSet is quite small — I'm wondering if we need it at all.)

@marctc
Copy link
Author

@marctc marctc commented Feb 14, 2017

Also, it should probably be filter_fields to match DRF.

I named it filterset_fields to keep coherence with filterset_class but you're right again.

Did you need this yourself? (The minimal FilterSet is quite small — I'm wondering if we need it at all.)

What do you mean?

@carltongibson
Copy link
Owner

@carltongibson carltongibson commented Feb 14, 2017

I mean, what prompted you to suggest this?

I see the point of it but, also it's not hard to define filterset_class.

I'm wondering if it's something you actually wanted in real usage?

@marctc
Copy link
Author

@marctc marctc commented Feb 14, 2017

We are building an ERP for a client and there a lot of models and a lot of FilterViews. Those views automatically renders the filter with all of the fields. If I want to exclude or explicity set a subset of fields, I have to define a filterset for each view. I'm a lazy developer, sorry :P

@carltongibson
Copy link
Owner

@carltongibson carltongibson commented Feb 14, 2017

That's not a problem. :-) OK. It's reasonable. If you can make the changes we can have it.

(I'm going to roll 1.0.2 this week, so it can join that.)

@carltongibson carltongibson added this to the 1.0.2 milestone Feb 14, 2017
@marctc
Copy link
Author

@marctc marctc commented Feb 14, 2017

Thanks! Working on it.

@marctc marctc changed the title Add filterset_fields and filterset_exclude to FilterView Add filter_fields to FilterView Feb 14, 2017
@marctc
Copy link
Author

@marctc marctc commented Feb 14, 2017

Done

@carltongibson
Copy link
Owner

@carltongibson carltongibson commented Feb 14, 2017

Awesome. Thanks.

@carltongibson carltongibson merged commit c168e9d into carltongibson:develop Feb 14, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants