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

Allow ChoiceField.choices to be set dynamically #5426

Merged
merged 1 commit into from Sep 20, 2017
Merged

Allow ChoiceField.choices to be set dynamically #5426

merged 1 commit into from Sep 20, 2017

Conversation

jpnauta
Copy link
Contributor

@jpnauta jpnauta commented Sep 17, 2017

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.
@tomchristie
Copy link
Member

tomchristie commented Sep 18, 2017

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.

@carltongibson carltongibson added this to the 3.6.5 Release milestone Sep 18, 2017
Copy link
Collaborator

@carltongibson carltongibson left a comment

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 carltongibson merged commit c0a4862 into encode:master Sep 20, 2017
1 check passed
@jpnauta
Copy link
Contributor Author

jpnauta commented Sep 20, 2017

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

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.

None yet

3 participants