-
Notifications
You must be signed in to change notification settings - Fork 170
chore: fixup operator type ignores for SQLExpr
#2920
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
Changes from all commits
40e1737
a8af821
f8e853c
fda9a7d
86ed14a
ec08949
a4de0bb
d372db6
3a97dcb
9a544e8
266cab9
4aae069
7ad112a
937e2a1
854e2f3
0fa44af
a2deceb
a9cabad
a2c4f31
dea1fe2
27e748f
495fb11
9383492
df82744
c65df29
dd39727
09731da
b4b48d9
a131d51
3521336
dcf1b80
e6f7640
5347b9f
b17d241
462cf8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -62,7 +62,17 @@ class NativeExpr(Protocol): | |||||||||||||||
| """ | ||||||||||||||||
|
|
||||||||||||||||
| def between(self, *args: Any, **kwds: Any) -> Any: ... | ||||||||||||||||
| def isin(self, *args: Any, **kwds: Any) -> Any: ... | ||||||||||||||||
|
|
||||||||||||||||
| # NOTE: None of these are annotated for `dx.Series`, but are added imperatively | ||||||||||||||||
| # Probably better to define a sub-protocol for `NativeSQLExpr` | ||||||||||||||||
| # - match `dx.Series` to `NativeExpr` | ||||||||||||||||
| # - match the others to `NativeSQLExpr` | ||||||||||||||||
| def __gt__(self, value: Any, /) -> Self: ... | ||||||||||||||||
| def __lt__(self, value: Any, /) -> Self: ... | ||||||||||||||||
| def __ge__(self, value: Any, /) -> Self: ... | ||||||||||||||||
| def __le__(self, value: Any, /) -> Self: ... | ||||||||||||||||
| def __eq__(self, value: Any, /) -> Self: ... # type: ignore[override] | ||||||||||||||||
| def __ne__(self, value: Any, /) -> Self: ... # type: ignore[override] | ||||||||||||||||
|
Comment on lines
+66
to
+75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks good, thanks - if anything, I like it better if we match on these than if we match on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the important bit narwhals/narwhals/_compliant/expr.py Lines 57 to 62 in 5bcdb6e
I had a similar issue for Line 124 in 0d4b6f7
It's okay to have multiple protocols/sub-protocols/aliases if we need that now I started with 1 because we had a common denominator, but we grow π There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
right, so if we add these comparison operators, we can remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keep Kind of like how we have Waaaaay more than just the types we're interested in will have comparison dunders, but fewer will also have |
||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| class CompliantExpr(Protocol[CompliantFrameT, CompliantSeriesOrNativeExprT_co]): | ||||||||||||||||
|
|
||||||||||||||||
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.
For context:
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.
oh my π€¦