Skip to content
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

Drop .choices from model field choices #369

Merged
merged 10 commits into from
Sep 24, 2023

Conversation

UnknownPlatypus
Copy link
Contributor

Fixes #336

This might break if someone designed a custom class with a choices property which is not a django enumeration class but I don't think this is really a use case ?
The documentation links are not yet valid, I think we have to play the waiting game when this MR seems ready for you.

Cheers

@adamchainz
Copy link
Owner

Hmmm. “I don't think this is really a use case?” is kinda shaky evidence. I agree it seems rather unlikely but maybe some people went out of their way to do so, perhaps copy-pasting in the old versions of the Choices classes as a backport, then never upgrading.

it’s probably okay, but this is also a very tiny improvement, especially since the choices attribute is being retained without deprecation.

hmm, I’m kinda split on this one.

@UnknownPlatypus
Copy link
Contributor Author

UnknownPlatypus commented Jul 24, 2023

perhaps copy-pasting in the old versions of the Choices classes as a backport, then never upgrading.

Indeed this is more likely. Maybe it's time to have some sort of configuration added to select rules ? I'm thinking of 2 possible ways of doing it:

  • Adding a way to select/exclude some fixers when running django-upgrade. This would also cover Add option to select specific fixer to run #333. We could also disable some maybe unsafe rules (like this one) by default and require user opt-in.
  • Adding some sort of applicability flag (--unsafe/--safe), we could then flag individual fixers as safe or not and only apply safe fixers by default. One could enable everything using the --unsafe flag. (I'm not sure I really like this option, I feel like It lacks some granularity compared to the other option)

it’s probably okay, but this is also a very tiny improvement, especially since the choices attribute is being retained without deprecation.

I agree this is a tiny improvement but since it has been added to django core, I think it's good to push this feature (and I do think it's more user-friendly from an API perspective.

Copy link
Owner

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for rebasing after I added the 5.0 support.

Returning to the conversation, I think this would actually be pretty safe. Looking again, I think you were probably right that the chances are slim that anyone actually has a custom class with .choices. Let’s go ahead as is and fix if anyone reports issues.

I have some comments about expanding the scope, if you wouldn't mind.

tests/fixers/test_model_field_choices.py Outdated Show resolved Hide resolved
src/django_upgrade/fixers/model_field_choices.py Outdated Show resolved Hide resolved
src/django_upgrade/fixers/model_field_choices.py Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@UnknownPlatypus
Copy link
Contributor Author

Thanks the thorough review, will look into it later this week 👌

Copy link
Owner

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, gonna do final edits and merge

tests/test_data.py Outdated Show resolved Hide resolved
@@ -36,7 +36,7 @@ def visit_ImportFrom(
parents: list[ast.AST],
) -> Iterable[tuple[Offset, TokenFunc]]:
if (
not state.looks_like_migrations_file
state.looks_like_models_file
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work. I think I'll actually split looks_like_models_file and this change into a separate PR.

CHANGELOG.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
tests/fixers/test_model_field_choices.py Outdated Show resolved Hide resolved
tests/fixers/test_model_field_choices.py Show resolved Hide resolved
@adamchainz adamchainz changed the title Drop model field choices Drop .choices from model field choices Sep 24, 2023
@adamchainz adamchainz merged commit 02976ae into adamchainz:main Sep 24, 2023
7 checks passed
@UnknownPlatypus UnknownPlatypus deleted the drop_model_field_choices branch December 9, 2023 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Django 5.0: Drop .choices for model field choices parameters
2 participants