-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[red-knot] Implement Type::Tuple
Comparisons
#13712
[red-knot] Implement Type::Tuple
Comparisons
#13712
Conversation
|
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.
This is really excellent, thank you!! A few comments.
884362d
to
860a8e6
Compare
cbf3c44
to
3dbdae8
Compare
In addition to the changes mentioned in your review, I have enhanced the inference for |
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.
Thanks! Some comments below:
crates/red_knot_python_semantic/resources/mdtest/comparison/tuples.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/comparison/tuples.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/comparison/tuples.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/comparison/tuples.md
Outdated
Show resolved
Hide resolved
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.
Thanks! This LGTM now, but I'd love a second approval from either @carljm or @dhruvmanila
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.
This looks good to me, modulo a few minor comment nits. Thanks for your work here!
I would like better test coverage (fewer todos). For example, right now the "unsupported comparison" case isn't really covered by the tests, because we don't have any rich-comparison operators implemented to return None
from infer_binary_type_comparison
for unsupported comparisons yet.
But the code here looks correct, so I'm happy to merge it as-is; the effective test coverage here will improve as we flesh out infer_binary_type_comparison
.
Summary
This PR implements comparisons for (tuple, tuple).
It will close #13688 and complete an item in #13618 once merged.
Test Plan
Basic tests are included for (tuple, tuple) comparisons.