Fix choices in ChoiceField to support IntEnum by b7wch · Pull Request #8955 · 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

Conversation27 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 }})

b7wch

Description

Python support Enum in version 3.4, but changed __str__ to int.__str__ until version 3.11 to better support the replacement of existing constants use-case. https://docs.python.org/3/library/enum.html#enum.IntEnum

rest_framework support Python 3.6+, this commit will support the Enum as datatype of choices in Field.

Discussions: #8945

auvipy

Choose a reason for hiding this comment

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

besides, this will need unit test coverage

@@ -1397,7 +1397,8 @@ def __init__(self, choices, **kwargs):
def to_internal_value(self, data):
if data == '' and self.allow_blank:
return ''
if isinstance(data, Enum) and str(data) != str(data.value):

Choose a reason for hiding this comment

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

from the test failures it seems that Enum is not defined. you might forgot to import it? or could you check what is the issue?

Choose a reason for hiding this comment

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

Thank you, I have added the import statement now.

Choose a reason for hiding this comment

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

I'm not sure why you are removing the django compatibility support codes here if isinstance(data, (IntegerChoices, TextChoices)) and str(data) !=
str(data.value):

auvipy

@@ -1397,7 +1398,8 @@ def __init__(self, choices, **kwargs):
def to_internal_value(self, data):
if data == '' and self.allow_blank:
return ''
if isinstance(data, Enum) and str(data) != str(data.value):

Choose a reason for hiding this comment

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

can you please add unit test coverage for the changes?

auvipy

Choose a reason for hiding this comment

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

Since the case where the enum values need to be integers is extremely common, Django provides an IntegerChoices class. For example:

class Card(models.Model):

class Suit(models.IntegerChoices):
    DIAMOND = 1
    SPADE = 2
    HEART = 3
    CLUB = 4

suit = models.IntegerField(choices=Suit.choices)

did you check this?

https://docs.djangoproject.com/en/3.2/ref/models/fields/#enumeration-types

@b7wch

Since the case where the enum values need to be integers is extremely common, Django provides an IntegerChoices class. For example:

class Card(models.Model):

class Suit(models.IntegerChoices):
   DIAMOND = 1
   SPADE = 2
   HEART = 3
   CLUB = 4

suit = models.IntegerField(choices=Suit.choices)

did you check this?

https://docs.djangoproject.com/en/3.2/ref/models/fields/#enumeration-types

Thanks for your suggestion, I use the class Enum implemented in python standard library first, and then I found that rest_framework is suitable for Enum.
Of course models.IntegerChoices could handle this situation too, but I think support Enum natively is also necessary.

@auvipy

this a django extension and should be using what django provides first

@auvipy

please use built in django feature here

@auvipy

closing in favor of #8968

@auvipy

@encode/django-rest-framework hello team, should we consider supporting python Enum directly beside the django built in ones?

@b7wch

@encode/django-rest-framework hello team, should we consider supporting python Enum directly beside the django built in ones?
closing in favor of #8968

Compare to #8968, I think my commit is more widely applicable

@auvipy

that is a django compat fix. If anyone want to support direct python enum, we need to rebase this PR :)

auvipy

Choose a reason for hiding this comment

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

can you please rebase it again?

@b7wch

can you please rebase it again?

@auvipy ok, I have rebased it.

auvipy

@@ -1397,7 +1397,8 @@ def __init__(self, choices, **kwargs):
def to_internal_value(self, data):
if data == '' and self.allow_blank:
return ''
if isinstance(data, Enum) and str(data) != str(data.value):

Choose a reason for hiding this comment

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

I'm not sure why you are removing the django compatibility support codes here if isinstance(data, (IntegerChoices, TextChoices)) and str(data) !=
str(data.value):

@@ -1414,11 +1411,8 @@ def to_internal_value(self, data):
def to_representation(self, value):
if value in ('', None):
return value
if isinstance(value, (IntegerChoices, TextChoices)) and str(value) != \

Choose a reason for hiding this comment

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

same goes for this. we are not going to remove django compatibility features.

Choose a reason for hiding this comment

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

I should make it clear.
if isinstance(value, (IntegerChoices, TextChoices)) and str(value) != \ in #8986 not resolve the problem, and this if judgment will never be executed with the IntegerChoices and TextChoices.
Because this problem only occurs in the python version little than 3.11 and use Enum as base class.

Choose a reason for hiding this comment

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

Add if isinstance(data, Enum) and str(data) != str(data.value): just enhances the Enum support for python version little than 3.11.
if isinstance(value, (IntegerChoices, TextChoices)) and str(value) != \ has not effect when users use IntegerChoices or TextChoices, because str(value) always equal to value.value according to 8740ff334ae3e001514c42ac1fb63a5e4cfa5cd1, but user swill still meet the problem when they use Enum, because that if judgment not solve the problem of IntEnum in different python version.

kevin-brown

@@ -1875,26 +1877,26 @@ def test_edit_choices(self):
field.run_validation(2)
assert exc_info.value.detail == ['"2" is not a valid choice.']
def test_integer_choices(self):
class ChoiceCase(IntegerChoices):
def test_enum_choices(self):

Choose a reason for hiding this comment

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

It would make sense to add this new test in addition to keeping the old IntegerChoices test just to ensure we don't have regressions in future versions of Django if they decide to do something special with that class.

@auvipy

Hey Burson! you were suggested to add new tests beside keeping the old tests using django specific class.

see:

It would make sense to add this new test in addition to keeping the old IntegerChoices test just to ensure we don't have regressions in future versions of Django if they decide to do something special with that class.

@b7wch

@b7wch

Hey Burson! you were suggested to add new tests beside keeping the old tests using django specific class.

see:

It would make sense to add this new test in addition to keeping the old IntegerChoices test just to ensure we don't have regressions in future versions of Django if they decide to do something special with that class.

@auvipy New test has added, and keeping the old tests.

auvipy

@auvipy

Thanks Burson! sorry for the initial over sight!

@b7wch

Thanks Asif Saif Uddin for your patient guidance and valuable insights.

math-a3k pushed a commit to math-a3k/django-rest-framework that referenced this pull request

Jul 31, 2023

@b7wch @math-a3k

@auvipy

hey, we might introduce some regressions, can you please check? #9388