Skip to content

Conversation

@rpkilby
Copy link
Collaborator

@rpkilby rpkilby commented Sep 21, 2016

No description provided.

template = loader.get_template(self.template)
return compat.template_render(template, context)

def get_fields(self, view):
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add a Doc string: what's this for? Ta.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The original blame points to core-api/schema generation, but I'm not familiar with that so I don't actually understand how this method is being used. @tomchristie?

Side note - I saw @kevin-brown's comment here and simplified.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, this is used by the schema generation to introspect what fields are required by a filter class. It's not formalized in the docs yet, so we should discuss prior to any 1.0 django-filter / 3.5 rest framework releases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tomchristie - could you take a look at 291717f? Does this seem correct? Thanks for the help.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems good to me.

return compat.template_render(template, context)

def get_fields(self, view):
filter_class = self.get_fitler_class()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

get_fitler_class()

And this is why we test anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I promise I can spell.

@rpkilby
Copy link
Collaborator Author

rpkilby commented Sep 23, 2016

@carltongibson. Anything else that needs to be done here? I'm considering holding off on a doc string until get_fields is public/final in DRF's API.

@lovelydinosaur
Copy link
Contributor

lovelydinosaur commented Oct 10, 2016

Okay, these will become public API in 3.5 onwards, as get_schema_fields.
Should return a list of coreapi.Field instances.

(see encode/django-rest-framework@3f3213b )

@carltongibson
Copy link
Owner

Awesome. Thanks Tom.

@rpkilby rpkilby force-pushed the backend-get-fields branch from 291717f to 0b1ec05 Compare October 16, 2016 23:06
@rpkilby rpkilby changed the title Port get_fields from DRF Port get_schema_fields from DRF Oct 16, 2016
@rpkilby rpkilby force-pushed the backend-get-fields branch from 86305ea to bc35978 Compare October 16, 2016 23:18
@rpkilby
Copy link
Collaborator Author

rpkilby commented Oct 17, 2016

Hi @carltongibson - this should be ready for review.

btw, there is technically a flaw with the implementation, but I don't think it's worth fixing. Given the following,

class UserFilter(FilterSet):
    username = filters.CharFilter()
    last_login__between = filters.DateTimeFromToRangeFilter(name='last_login', lookup_expr='range')

    class Meta:
        model = User

get_schema_fields will incorrectly return query names of ['username', 'last_login__between'] instead of ['username', 'last_login__between_0', 'last_login__between_1'].

Realistically, this is only an issue for MultiWidget, although it could technically apply to any widget. In general, I'm not inclined to try to make this compatible:

My thoughts are to either:

  • Not worry about it until it becomes relevant.
  • Add a note in the docs that some MultiWidget-based filters aren't compatible with all of DRF's features. Instead of using the *RangeFilters, create a CSV-based filter derived from BaseRangeFilter. Either that or create, two separate lte/gte filters (see this thread).

@carltongibson carltongibson added this to the 0.15.3 milestone Oct 17, 2016
@carltongibson
Copy link
Owner

OK. This is great. I'll merge this and push a 0.15.3 this morning. 👍

@tomchristie I think we're good from this side for DRF 3.5.

@carltongibson carltongibson merged commit 7a9caa6 into carltongibson:develop Oct 17, 2016
@rpkilby rpkilby deleted the backend-get-fields branch October 17, 2016 07:20
@lovelydinosaur
Copy link
Contributor

Great stuff!

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.

3 participants