.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.

[UnknownPlatypus]

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.

I'm not sure it's worth because it would create inconsistency across the codebase, with people wondering why sometimes there is a .choices and sometimes not.

To handle this safely all the time, it would require multi-file analysis and type inference which is way to much work for this fixer. Maybe something a tool like ruff might be able to support at some point.

Given the very little gain in convenience and readability, I don't think it's worth the hassle.
What do you think @adamchainz ?

[adamchainz]

I'm not sure it's worth because it would create inconsistency across the codebase, with people wondering why sometimes there is a .choices and sometimes not.

I'm not so concerned about this. There are lots of edge cases django-upgrade backs out of handling because not everything can be automated.

To handle this safely all the time, it would require multi-file analysis and type inference which is way to much work for this fixer. Maybe something a tool like ruff might be able to support at some point.

Yes, agree.

Given the very little gain in convenience and readability, I don't think it's worth the hassle.
What do you think @adamchainz ?

I'm happy leaving this open for now, in case someone wants to retry it with the extra guards. I think it would be nice and help advertise the ability to skip .choices.

[UnknownPlatypus]

Alright, your probably right. I might take a stab at this this week then