Fix choices in ChoiceField to support IntEnum #8955

b7wch · 2023-04-20T03:42:26Z

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

besides, this will need unit test coverage

auvipy · 2023-04-20T04:07:02Z

rest_framework/fields.py

@@ -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):


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?

Thank you, I have added the import statement now.

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 · 2023-04-23T05:28:55Z

rest_framework/fields.py

@@ -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):


can you please add unit test coverage for the changes?

auvipy

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 · 2023-04-24T06:32:41Z

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 · 2023-04-25T07:36:59Z

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

auvipy · 2023-04-30T09:10:59Z

please use built in django feature here

auvipy · 2023-05-02T00:59:48Z

closing in favor of #8968

auvipy · 2023-05-02T11:56:18Z

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

b7wch · 2023-05-09T07:53:04Z

@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 · 2023-05-09T14:14:36Z

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

auvipy

can you please rebase it again?

b7wch · 2023-07-06T07:53:38Z

can you please rebase it again?

@auvipy ok, I have rebased it.

auvipy · 2023-07-08T04:59:48Z

rest_framework/fields.py

@@ -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):


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 · 2023-07-08T05:05:02Z

rest_framework/fields.py

@@ -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) != \


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

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.

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.

rest_framework/fields.py

kevin-brown · 2023-07-08T05:19:38Z

tests/test_fields.py

@@ -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):


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 · 2023-07-13T07:07:47Z

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.

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](https://docs.python.org/3/library/enum.html#enum.IntEnum) rest_frame work support Python 3.6+, this commit will support the Enum in choices of Field.

b7wch · 2023-07-13T08:12:06Z

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 · 2023-07-13T12:51:27Z

Thanks Burson! sorry for the initial over sight!

b7wch · 2023-07-13T13:37:14Z

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

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](https://docs.python.org/3/library/enum.html#enum.IntEnum) rest_frame work support Python 3.6+, this commit will support the Enum in choices of Field.

auvipy · 2024-05-29T05:53:18Z

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

auvipy requested changes Apr 20, 2023

View reviewed changes

b7wch force-pushed the enum_choice branch 2 times, most recently from 3ca5d08 to b8926b0 Compare April 20, 2023 04:34

b7wch requested a review from auvipy April 23, 2023 03:51

auvipy requested changes Apr 23, 2023

View reviewed changes

auvipy added this to the 3.15 milestone Apr 23, 2023

b7wch force-pushed the enum_choice branch from b8926b0 to 56eca25 Compare April 24, 2023 03:13

auvipy added Enhancement Bug labels Apr 24, 2023

auvipy requested changes Apr 24, 2023

View reviewed changes

saadullahaleem mentioned this pull request May 1, 2023

Fix mapping for choice values #8968

Merged

auvipy closed this May 2, 2023

auvipy reopened this May 2, 2023

auvipy requested changes Jun 18, 2023

View reviewed changes

b7wch force-pushed the enum_choice branch 3 times, most recently from 8999b29 to a4e1038 Compare July 6, 2023 07:24

auvipy requested changes Jul 8, 2023

View reviewed changes

kevin-brown reviewed Jul 8, 2023

View reviewed changes

b7wch force-pushed the enum_choice branch from a4e1038 to 2f10051 Compare July 13, 2023 02:34

b7wch force-pushed the enum_choice branch from 2f10051 to 93a4006 Compare July 13, 2023 07:46

b7wch force-pushed the enum_choice branch 2 times, most recently from 6086601 to b043df8 Compare July 13, 2023 08:00

b7wch force-pushed the enum_choice branch from b043df8 to 75bb41f Compare July 13, 2023 08:00

auvipy approved these changes Jul 13, 2023

View reviewed changes

auvipy merged commit 66d86d0 into encode:master Jul 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix choices in ChoiceField to support IntEnum #8955

Fix choices in ChoiceField to support IntEnum #8955

b7wch commented Apr 20, 2023 •

edited

Loading

auvipy left a comment

auvipy Apr 20, 2023

b7wch Apr 20, 2023

auvipy Jul 8, 2023

auvipy Apr 23, 2023

auvipy left a comment

b7wch commented Apr 24, 2023 •

edited

Loading

auvipy commented Apr 25, 2023

auvipy commented Apr 30, 2023

auvipy commented May 2, 2023

auvipy commented May 2, 2023

b7wch commented May 9, 2023 •

edited

Loading

auvipy commented May 9, 2023

auvipy left a comment

b7wch commented Jul 6, 2023

auvipy Jul 8, 2023

auvipy Jul 8, 2023

b7wch Jul 13, 2023

b7wch Jul 13, 2023

kevin-brown Jul 8, 2023

auvipy commented Jul 13, 2023

b7wch commented Jul 13, 2023

auvipy commented Jul 13, 2023

b7wch commented Jul 13, 2023

auvipy commented May 29, 2024

Fix choices in ChoiceField to support IntEnum #8955

Fix choices in ChoiceField to support IntEnum #8955

Conversation

b7wch commented Apr 20, 2023 • edited Loading

Description

auvipy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auvipy left a comment

Choose a reason for hiding this comment

b7wch commented Apr 24, 2023 • edited Loading

auvipy commented Apr 25, 2023

auvipy commented Apr 30, 2023

auvipy commented May 2, 2023

auvipy commented May 2, 2023

b7wch commented May 9, 2023 • edited Loading

auvipy commented May 9, 2023

auvipy left a comment

Choose a reason for hiding this comment

b7wch commented Jul 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auvipy commented Jul 13, 2023

b7wch commented Jul 13, 2023

auvipy commented Jul 13, 2023

b7wch commented Jul 13, 2023

auvipy commented May 29, 2024

b7wch commented Apr 20, 2023 •

edited

Loading

b7wch commented Apr 24, 2023 •

edited

Loading

b7wch commented May 9, 2023 •

edited

Loading