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

Lookup should be a subtype of Expression #2199

Merged

Conversation

mkurnikov
Copy link
Member

I have made things!

Added test to trigger the type failure.
Had to change signature for set_source_expressions to Combinable | Expression, and remove some inherited methods.

@sobolevn

@mkurnikov mkurnikov requested a review from sobolevn May 30, 2024 16:40
@mkurnikov mkurnikov merged commit 42ddece into typeddjango:master May 30, 2024
36 checks passed
@mkurnikov mkurnikov deleted the lookup-inherited-from-expression branch May 30, 2024 21:24

CheckConstraint(
name="less_than_constraint",
check=LessThan(F("months"), 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since mypy typecheck tests are slow, and we also want to test with Pyright, such tests are better added under tests/assert_type

Copy link
Member Author

@mkurnikov mkurnikov May 31, 2024

Choose a reason for hiding this comment

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

Thanks, I should've waited for your review before merging.
I'll fix it in a follow-up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No problem, this is more of a minor quibble. Follow-up PR is welcome. :)

Welcome back to django-stubs development @mkurnikov! 🎉

Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW @flaeppe already changed this #2204

Copy link
Member Author

Choose a reason for hiding this comment

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

Great, thanks @flaeppe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants