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.

Sign up for GitHub

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

Jump to bottom

Update models's fields' _Choices type #2476

Merged
merged 1 commit into from
Feb 7, 2025
Merged

Update models's fields' _Choices type #2476

merged 1 commit into from
Feb 7, 2025

Conversation

stianjensen
Copy link
Contributor

Django 5 allows model field choices to be callables, mappings or subclasses of models.Choices. This commit introduces these options in the stubs.

Continuation of #2368, to fix a merge conflict, avoid issues with importing Tuple instead of using tuple, and avoid unnecessary formatting changes.

@stianjensen stianjensen mentioned this pull request Jan 9, 2025
Closed
sobolevn
sobolevn reviewed Jan 9, 2025
View reviewed changes
@stianjensen stianjensen force-pushed the django5-choices branch 2 times, most recently from c104ea7 to 3531bd4 Compare January 9, 2025 21:46
sobolevn
sobolevn reviewed Jan 9, 2025
View reviewed changes
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks! I would like to have one more pair of eyes :)
cc @intgr @flaeppe

@sobolevn sobolevn mentioned this pull request Jan 10, 2025
Merged
@stianjensen
Copy link
Contributor Author

I see you made another release now – would be great if this made the cut for the next one!

sobolevn
sobolevn reviewed Feb 7, 2025
View reviewed changes
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

The only question I have is about type params to fields.

@stianjensen
Copy link
Contributor Author

Yeah good point, that shouldn't be necessary. Updated!

sobolevn
sobolevn reviewed Feb 7, 2025
View reviewed changes
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Sorry, I've missed to comment on them during the prev review :(

tests/assert_type/db/models/fields/test_choices.py Outdated
FIRST = 1, "bar"
SECOND = 2, "bar"

char1 = models.CharField[str, str](max_length=5, choices=TextChoices, default="foo")
Copy link
Member

Choose a reason for hiding this comment

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

The same question about these ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah, no worries – I could have looked through for those myself, too!

@stianjensen
Copy link
Contributor Author

But this last change seems to break pyright tests?

@sobolevn
Copy link
Member

sobolevn commented Feb 7, 2025

Ok, that's unfortunate. Let's move this test to mypy test only, because we don't officially support pyright and I don't know anything about how it works :(

@stianjensen
Copy link
Contributor Author

Cool! How do I make a test mypy only? 😅

@sobolevn
Copy link
Member

sobolevn commented Feb 7, 2025

You move it to tests/typecheck/db/models/test_fields.yml

@joaoseckler @stianjensen
Update models's fields' _Choices type
c473f88 
Django 5 allows model field choices to be callables, mappings or
subclasses of models.Choices. This commit introduces these options in
the stubs.

Co-Authored-By: Stian Jensen <[email protected]>
@stianjensen
Copy link
Contributor Author

Thanks for the help and patience!

Here's a new attempt, with proving a couple invalid cases first, then all the succesful asserts, all in a yaml test case for mypy!

sobolevn
sobolevn approved these changes Feb 7, 2025
View reviewed changes
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks!

@sobolevn sobolevn merged commit bda1956 into typeddjango:master Feb 7, 2025
40 checks passed
@stianjensen stianjensen deleted the django5-choices branch February 7, 2025 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sobolevn sobolevn sobolevn approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stianjensen @sobolevn @joaoseckler