Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Should choice values be enumeration instances? #124

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

Closed
bckohan opened this issue Apr 29, 2025 · 1 comment
Closed

Should choice values be enumeration instances? #124

bckohan opened this issue Apr 29, 2025 · 1 comment
Assignees
Labels
BREAKING A change that is not backwards compatible. enhancement New feature or request question Further information is requested
Milestone

Comments

@bckohan
Copy link
Member

bckohan commented Apr 29, 2025

Impacted attributes: Field.flatchoices and Field.choices.

Motivation: #123

This would mostly be about removing the hash equivalency requirement on enum types for 100% interface compatibility. The only known thing in core this fixes is the list display in the admin for non-hash equivalent Enum types.

Would need to also consider how this would affect migrations.

I expect this would have a lot of breaking impacts in other parts of the code, so right now my null is that we should not do this. It does warrant more investigation though on the 3.0 timeline.

@bckohan bckohan added BREAKING A change that is not backwards compatible. enhancement New feature or request question Further information is requested labels Apr 29, 2025
@bckohan bckohan added this to the 3.0 milestone Apr 29, 2025
@bckohan bckohan self-assigned this Apr 29, 2025
@kharybdys
Copy link

(Continuing the discussion from #123)

From my viewpoint, the only reason to want to advise enum instances to be hash equivalent to their values is because Django choose completely the wrong architecture for their Choices.

It has annoyed me to no end that their TextChoices etc do not just subclass Enum. Why do I think it's the wrong architecture: They were basically trying to make a three-valued Enum (name, value, display_value/key) versus the standard two-valued Enum (name, value). Principally, for display in eg the Admin interface one should encode at most a display_key in the code and use the I18n mechanism to generate the display_value from that. I thus don't even get why they thought they'd need something beyond an Enum. The most logical display_key would be <enum_class_name>..

Their admin interface handles the third value by using a dict internally to translate the Enum value into the display_value/key and(!) they use the raw value of the field and not the Python value for it, or your methods would've worked by specifying choices as Enum_instance <-> display value. From this comes the need for the raw value in the field to be hash equivalent to the enum instance.

Now, given that Django will not be changing this, the question is whether it is correct to advise hash-equivalency. I actually think it is flat out wrong, because it very much suggests (but not guarantees) that if values are hash equivalent, they'd also return equals = True. And that is just not the intent of using a pure Enum, where the Enum instance is not at all equivalent to their value.

You suggested that switching to choices being Enum_instance <-> display value would cause trouble with (de)serialization all over the place. But I don't follow you on that. If you know something is an Enum, the obvious way to (de)serialize is Enum.value for the one way and Enum(value) for the other way. Nothing in there says anything about requirements on hashes. It might be worthwhile to check what eg. Pydantic is doing but I would be surprised if it requires in anyway some specific thing about hashes.

So, I conclude that the correct choice is choices being Enum_instance <-> display value. However, this is the conclusion in the perfect world, the ivory tower etc. Practically speaking I must confess that I have no clue what problems this change would be causing.

I did suggest to only re-implement flatchoices and leave choices alone as a way to mitigate the risk of the change as from a first glance I gathered that flatchoices is only used for display purposes. It might be that the whole choices tuple is only used for display purposes (/validation of acceptable values) though in which case it might even be feasible to provide choices as Enum_instance <-> display value.

I want to finish with stating that I'm just a passer-by. I'm very happy with your library and I don't mind implementing any of the work-arounds listed in #123. I do not have to do the work of making the change etc so in the end you have to make the choice that is not just right for this library but for you as a maintainer.

@django-commons django-commons locked and limited conversation to collaborators May 2, 2025
@bckohan bckohan converted this issue into discussion #126 May 2, 2025

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
BREAKING A change that is not backwards compatible. enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants