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 a method to return the string value used in choices of a RelatedField #3255

Merged
merged 5 commits into from
Aug 10, 2015
Merged

Add a method to return the string value used in choices of a RelatedField #3255

merged 5 commits into from
Aug 10, 2015

Conversation

jamesbeith
Copy link
Contributor

Issue #3254

Returns the text representation of the instance. Subclasses can override this method to provide a different display value used for populating the `choices` property.
class TrackPrimaryKeyRelatedField(serializers.PrimaryKeyRelatedField):
def display_value(self, instance):
return 'Track: %s' % (instance.title)

Copy link
Member

Choose a reason for hiding this comment

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

Okay, great! I think it'd be worth finally noting that...

"These string representations will be used in select HTML inputs in the browsable API"

or similar. Just to make it absolutely clear when and why you would choose to override this method.

@jamesbeith
Copy link
Contributor Author

I've update the docs with your suggestion. Just to note, GitHub's source diff for that Markdown file is completely wrong for me for both commits, however viewing the rich diff displays the changes correctly.

@tomchristie
Copy link
Member

I think drop duplicated six.text_type on L151, right?

@tomchristie
Copy link
Member

Since we already do that in the method.

@jamesbeith
Copy link
Contributor Author

Possibly. It depends how strictly you define the API. Is it SHOULD or MUST return a string value.

Could there be a use case where someone is iterating the field's queryset in some of their own logic and calling the display_value method independently with one/some/all of the instances? If so then we should guarantee we return them a string (of course their own subclasses could break this contract). It could be a rare case I admit. Happy to remove it if you think it's not necessary.

@tomchristie
Copy link
Member

I'd leave it down to the user to deal with. It'll be used in template code so it'll get coerced in any case.

@jamesbeith
Copy link
Contributor Author

Ah apologies, I originally read that wrong, I was looking at L161. Yes to removing it from L151.

The `display_value` method returns a text type.
@tomchristie tomchristie added this to the 3.2.2 Release milestone Aug 10, 2015
@tomchristie
Copy link
Member

Lovely stuff!

tomchristie added a commit that referenced this pull request Aug 10, 2015
Add a method to return the string value used in `choices` of a `RelatedField`
@tomchristie tomchristie merged commit 2d5e8de into encode:master Aug 10, 2015
@jamesbeith jamesbeith deleted the issues/#3254-display-value branch August 11, 2015 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants