-
Notifications
You must be signed in to change notification settings - Fork 0
Support type aliases, NamedTuple
and TypedDict
in constrained TypeVar defaults
#2
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
base: master
Are you sure you want to change the base?
Conversation
…8862-typevar-default-alias
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Thank you, I tried your changes locally and I believe it also fixes python#17686. |
Missed that one, thanks! |
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
1 similar comment
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
WalkthroughThe changes update the logic for checking if a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MypyChecker
participant TypeVar
participant is_same_type
User->>MypyChecker: Define TypeVar with constraints and default
MypyChecker->>TypeVar: Check if default is among constraints
TypeVar->>is_same_type: Compare default with each constraint
is_same_type-->>TypeVar: Return comparison result
TypeVar-->>MypyChecker: Valid/invalid default determination
MypyChecker-->>User: Accept or report error
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🧰 Additional context used🧬 Code Graph Analysis (1)mypy/checker.py (1)
🔇 Additional comments (9)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
@coderabbitai full review |
✅ Actions performedFull review triggered. |
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request addresses an issue where TypeVar defaults were not being correctly checked against their constraints, especially when type aliases, NamedTuple, or TypedDict were involved. The changes include modifications to the checker and checkexpr modules to use is_same_type
for comparing types, and the addition of new test cases to cover various scenarios. Overall, the changes seem well-structured and address the reported issues effectively.
Summary of Findings
- Type comparison: The change from
==
tois_same_type
ensures that type aliases and special types are correctly compared when checking TypeVar defaults against constraints. This is crucial for the correctness of type checking. - Test coverage: The addition of new test cases covering type aliases, NamedTuple, and TypedDict significantly improves the test coverage for TypeVar defaults and constraints.
Merge Readiness
The pull request appears to be well-structured and addresses the reported issues effectively. The addition of new test cases significantly improves the test coverage. I recommend merging this pull request after addressing the review comments. I am unable to directly approve the pull request, and users should have others review and approve this code before merging.
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request addresses issues related to type aliases, NamedTuple
, and TypedDict
in constrained TypeVar
defaults. The changes involve modifications to the checker and checkexpr modules to improve type checking accuracy, along with the addition of new test cases to ensure the correct handling of these features. Overall, the changes seem well-structured and contribute to the stability and correctness of the type checker.
Summary of Findings
- Severity Levels: The review settings requested that review comments be provided for issues at the
medium
,high
, andcritical
levels of severity. No issues at these levels were found.
Merge Readiness
The pull request appears to be in good shape for merging. The code changes are focused and address the specified issues. The addition of comprehensive test cases further increases confidence in the correctness of the implementation. Since no issues at the critical
, high
, or medium
levels of severity were found, the pull request seems ready to be merged, though as always, another reviewer should look over the changes before merging. I am unable to directly approve the pull request, and that users should have others review and approve this code before merging.
Fixes python#18862. Fixes python#17686.
Summary by CodeRabbit
Bug Fixes
Tests