Allow ChoiceField.choices to be set dynamically by jpnauta · Pull Request #5426 · encode/django-rest-framework (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation3 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Description

The choices field for the ChoiceField class should be able to be edited after __init__ is called.

field = ChoiceField(choices=[1,2])
field.choices = [1]  # Should no longer allow `2` as a choice

Currently, you must update choices, grouped_choices, and choice_strings_to_values to achieve this. This P/R keeps grouped_choices and choice_strings_to_values in sync whenever the choices are edited.

Description

The choices field for the ChoiceField class should be able to be edited after ChoiceField.__init__ is called.

field = ChoiceField(choices=[1,2])
field.choices = [1]  # Should no longer allow `2` as a choice

Currently, you must update choices, grouped_choices, and choice_strings_to_values to achieve this. This P/R keeps grouped_choices and choice_strings_to_values in sync whenever the choices are edited.

Seems okay, yes. No particular reason why you should instead just override the entire field, but we could go with this? Any issues that I might be missing that this could quietly introduce, anyone?

@encode/django-rest-framework-core-team Welcome to merge if anyone else okays this too.

Choose a reason for hiding this comment

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

This looks OK to me. (A few days mulling it and I still can't see an issue; tests pass; ...) So lets go.

Aside: I'd be inclined towards a follow-up that just made group_choices and choice_strings_to_values calculated properties. It strikes me we're storing 3 bits of state where 1 would suffice. Anyhoo...

@carltongibson I debated that design too; I can make another P/R to change group_choices and choice_strings_to_values into calculated properties if you'd like 😄