`.choices` dropped for choices not subclassing the `*Choices` class

[niccolomineo]

Python Version

3.12.1

Django Version

5.0.1

Package Version

1.15.0

Description

I have a number of choice classes in a parent-child relationship. Such a relationship is the reason why, in those classes, I am not inheriting from any Django *Choices class.

I am using these choice classes in choices field params and django-upgrade appears to be dropping the .choices attribute since it is supporting Django 5.0. Passing that attribute is vital for such choice classes to work, though.

Shouldn't django-upgrade detect whether the passed .choices belongs to a class inheriting from any Django *Choices class, and only in that case drop the .choices attribute?

[adamchainz]

Shouldn't django-upgrade detect whether the passed .choices belongs to a class inheriting from any Django *Choices class, and only in that case drop the .choices attribute?

In general, django-upgrade can't do that as the AST is insufficient to determine the class. It could apply in niche cases.

We discussed this possibility in #369, and I guess it was probably rare enough not to occur. Well, here we are with a report that it happened. I think we should probably revert the feature. It's not that big of a fix anyway.

What do you think @UnknownPlatypus ?

[UnknownPlatypus]

The easy solution is probably to revert the feature I agree.

But In the meantime, they will be some people with codebase they have control over knowing this fixer is safe for them. Maybe we could reconsider the idea of having some fixers disabled by default and you'd have to opt-in for them if you know they are safe for you use case ? (I briefly gave some ideas in this comment)

Another option would be to track TextChoices/IntegerChoices definitions in the file and only apply fix in these case but it adds some complexity and makes the fixer less predictable for people.

Let me know what you prefer, I'll be happy to help anyways.

[washeck]

I just want to add that I was also hit by this. Our codebase is full of classes like this one

class ReportStatus:
    IN_PROGRESS = "IN_PROGRESS"
    COMPLETED = "COMPLETED"
    choices = ((IN_PROGRESS, _("In progress")), (COMPLETED, _("Completed")))

which we used before TextChoices existed and removal of the .choices attribute broke our code.

[adamchainz]

I’ve reverted the fixer for now in #425, released in 1.16.0. But @UnknownPlatypus if you want to limit it to definitions within the same module, I think that would be a good compromise. It’s likely to fix many cases but not all.

[niccolomineo]

I’ve reverted the fixer for now in #425, released in 1.16.0. But @UnknownPlatypus if you want to limit it to definitions within the same module, I think that would be a good compromise. It’s likely to fix many cases but not all.

Thanks! As a long term fix, what about checking the inheriting class to tell whether .choices is needed?

[adamchainz]

Yes, as long as the choices class exists within the same module.