Fixed #30902 -- Added __str__() for model choice enums.

[carltongibson]

Allows expected comparison against strings, matching behaviour of created
instances with those fetched from the DB.

@shaib @pope1ni — This looks right to me, especially given the example on the ticket of the difference between created and retrieved instances but, what do you think? Thanks!

[ngnpope]

This looks right to me, especially given the example on the ticket of the difference between created and retrieved instances but, what do you think?

I thought there was a reason that we chose not to do this, but I'm struggling to remember the reasoning now. I must say, however, this wouldn't be the only case I've come across where values attached to an object that has been saved are not equivalent to the value that is populated from the rows returned from the database.

I have no objection if it generally works without breaking anything else - we're already being rather strict with how these enums are defined and used to ensure that things play nicely.

[carltongibson]

… I'm struggling to remember the reasoning now.

Yeah, that's where I am right now. 😀

[NyanKiyoshi]

Do we have a test as well to ensure the IntegerChoices also doesn't return a enum value but an integer? Not sure how achievable it would be.

[charettes]

Not sure how achievable it would be.

By defining IntegerChoices.__int__?

This makes me wonder if this __str__ should be defined on TextChoices instead?

[NyanKiyoshi]

By defining IntegerChoices.__int__?

Didn't know about this method's existence 👍

[ngnpope]

Hmm... This is where I get a little dubious about this. If we define TextChoices.__str__() and IntegerChoices.__int__(), what happens for other subclasses of Choices, e.g. one that uses datetime.date as a concrete type?

[shaib]

Two comments:

First of all, we do not need to take care of any type other than str.

@NyanKiyoshi said:

Do we have a test as well to ensure the IntegerChoices also doesn't return a enum value but an integer?

but this is a false dichotomy; an instance of IntegerChoices is both an enum value and an integer -- IntregerChoices is a subclass of int.

Secondly, the conversion of a TextChoices instance to str came up during the original work on the feature. @jrief argued that it should yield the label, others argued that it should return the value like this PR suggests. I explained in #11223 (comment) why calling str() on a TextChoices[1] instance is ambiguous, and in the spirit of my favorite Zen aphorism, suggested that we raise an exception. This is a bit problematic, because it could break the string representation of queries, or force us to do, in some contexts, weird things like

   x if isinstance(x, str) else str(x)

where we currently do the intuitive str(x).

All in all, I think the current behavior is the least of possible evils, and the issue should be solved by documentation, not behavior change.

[1] The names "Choices", "TextChoices" etc. came later; the comment refers to it as "ChoiceStrEnum".

[shaib]

On second thought, having looked at the ticket again, it may make sense to introduce a behavior change, but in CharField rather than TextChoices -- to make sure that once a value has been put in the field, it is an instance of str and not a subclass. I see value in removing the ambiguity at that point.

[carltongibson]

Thanks @shaib for your input. (See bottom) I haven't had the time yet to think through your last suggestion, but I wanted to comment with what I've got thus far, since I don't think we can settle with the current situation (that I know you since re-commented):

All in all, I think the current behavior is the least of possible evils, and the issue should be solved by documentation, not behavior change.

From the ticket:

This issue only arises if one explicitly calls str() on the enum value. Otherwise, as the supplied test shows, the value is an instance of str. As such, it serializes e.g. to JSON correctly with json.dumps(), so I don't see an issue with external APIs either.

OK so...

>>> v = my_object.my_str_value
>>> v
<MyChoice.FIRST_CHOICE: 'first'>
>>> isinstance(v, str)
True
>>> import json
>>> json.dumps(v)
'"first"'

But, every other usage is not expected:

>>> "%s" % v
'MyChoice.FIRST_CHOICE'
>>> print(v, str(v))
MyChoice.FIRST_CHOICE MyChoice.FIRST_CHOICE

OK, we can work around that. Ultimately we can say, "but they're Enums, and
that's how Enums behave":

>>> v.value
'first'

This is the "the issue should be solved by documentation, not behavior change" alternative.

But that's not how people use model values. That they're Enums is an
implementation detail.

We can't saddle that on our users, who are doing this:

>>> from django.template import Context, Template
>>> t = Template("Will '{{ enum_value }}' render how users expect it to?")
>>> c = Context({"enum_value": v })
>>> t.render(c)
"Will 'MyChoice.FIRST_CHOICE' render how users expect it to?"

Recall though that, v is the value of a model instance attribute, so it looks more like:

>>> c = Context({"enum_value": my_object.my_str_value })
>>> t.render(c)
"Will 'MyChoice.FIRST_CHOICE' render how users expect it to?"

Or really this:

>>> t = Template("Will '{{ object.my_str_value }}' render how users expect it to?")
>>> c = Context({"object": my_object })
>>> t.render(c)
"Will 'MyChoice.FIRST_CHOICE' render how users expect it to?"

That's (techical term :) broken.

So from the super helpful #11223 (comment)

We could take another variant of @Krolken's suggestion, and force str() to
return the value on all ChoiceEnums. This would make all ChoiceEnums
different from other Enums in a surprising way.

At this point, I'd say Yes to that.

IIRC the objections on the mailing list to having Enums (at all) were that their behaviour wasn't helpful enough, so, in order to get past that, we opted to adjust that behavioiur. This, then, is just more in that vein.

We subclass Enum to make it suitable for use as a Django model attribute choice. That its str behaviour is different from the base class is OK, on that line. (We can call-out that difference in the docs.)

However...

it may make sense to introduce a behavior change, but in CharField rather than TextChoices -- to make sure that once a value has been put in the field, it is an instance of str and not a subclass.

Not sure yet what this entails, so could be a possibility. (Can't predict instantly what it would look like, or what other issues it would throw up...)

Thoughts?

[ngnpope]

Thanks for your analysis @carltongibson.

So we tried to keep things as close to the standard enum as possible, but - as I mentioned earlier - we have had to make some choices to ensure things behave as expected, e.g. enforcing unique members, adding display labels. This is also why I steered away from naming such as ChoiceStrEnum in favour of TextChoices as these are explicitly intended for use with Django model fields that support choices. Support for free-form enums without any constraints on their use would just be asking for trouble - undoubtedly why the ticket was originally rejected the first time, despite the clear benefits to managing choices that this implementation now provides.

So looking back at the options in #11223 (comment) which can be summarised as the following:

1. Avoid defining a str-typed subclass of Choices.
2. Make Choices.__str__() return .label.
3. Make TextChoices.__str__() return .value.
4. Make Choices.__str__() return .value.
5. Make TextChoices.__str__() raise an exception.

I'd also add these additional options for consideration:

6. Introduce a behavior change, but in CharField (as suggested by Shai).
7. Introduce a behavior change in all Fields to disallow enum members - force use of .value.

Option 1 is not viable as we need to have concrete types defined - it solves so many other issues related to comparison, etc. Option 2 doesn't make sense to me - it would conflict with the problem that we have at the moment anyway. Options 3 and 5 would leave a discrepancy between TextChoices and other subclasses of Choices which would still return the string representation of the qualified name of the enum member and not the string representation of the value. In addition, option 5 would not interact well with the issues raised by Carlton with respect to rendering in templates.

I'm not keen on option 6 - that sounds like transparently calling .value for enum members in CharField which is prone to breakage in other subclasses, and I think we'll be chasing this problem around in other obscure parts of the ORM. Option 7 is consistent for all subclasses of Choices but forces people to be explicit; with the disadvantage that we have to work out every place that we might pass the value - the same problem.

In light of this, I think that we're stuck with option 4 - to change Choices.__str__() to return str(self.value) - and it should be on Choices, not just TextChoices, so that we return a consistent representation irrespective of the concrete type used - that is the string representation of the value.

I think we just need to now accept the fact that we are not providing support for generic enums - we are providing a Choices type that is enum-like and enum-backed, but has to make its own rules - after all, if generic enums would even work we'd not have had to implement all this in the first place!

[ngnpope]

After thinking through all the options I feel this is the only suitable solution. Thanks @carltongibson!

[shaib]

Yes, @carltongibson @pope1ni I'm convinced. Thanks!

[carltongibson]

OK, thank you both. Do we need to call something out in the docs? (I shall look... — but any input welcome. 😀)

[shaib]

I don't think anything is needed in the docs, but I'd put a short summary of why we define `__str__()` this way in comments in the code.

[ngnpope]

There is a bulleted list of differences to standard enums in the documentation that you could add something to, but then again we don't mention the special handling of __contains__. Am happy with a code comment.

[carltongibson]

OK, thanks again both. I'm just doing something else, then I'll come back and adjust this.

[felixxm]

Thanks y'all for a great discussion 🚀