-
Notifications
You must be signed in to change notification settings - Fork 170
fix(typing): Avoid overlapping DataFrame, LazyFrame
#2944
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
Conversation
- 76 errors in `tests/frame/join_test.py` - stable errors are expected, as they aren't using updated typing yet
Still same issues on `join`, `v1`, `v2`
May these can be typed *some day*, but they were never safe to begin with
|
These |
| EagerSeriesT, EagerExprT, NativeDataFrameT, "DataFrame[NativeDataFrameT]" | ||
| ], | ||
| CompliantLazyFrame[EagerExprT, NativeDataFrameT, "DataFrame[NativeDataFrameT]"], | ||
| CompliantLazyFrame[EagerExprT, "Incomplete", "DataFrame[NativeDataFrameT]"], |
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.
Pretty sure this Incomplete is unavoidable, just like the [type-var] warnings on v1.DataFrameLike
narwhals/narwhals/stable/v1/__init__.py
Lines 96 to 98 in 89095ce
| # NOTE legit | |
| class DataFrame(NwDataFrame[IntoDataFrameT]): # type: ignore[type-var] | |
| _version = Version.V1 |
I'm much happier to eat those two, when it fixes sooooooooo much else π₯³
|
@MarcoGorelli I just wanna loop you into this one early, since it may be a challenge to keep conflict-free π
Mainly in terms of where we no longer need/now require type ignore(s) |
|
i think this is fine, yes, thanks! |
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.
@dangotbanned this seems pretty safe to me but I am still not confident enough to approve a PR solely aiming at improving the typing experience!
So let me play devil's advocate: do you see any risk or anything that can be problematic?
Also I left one question in a test file
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.
Fixed during the series of PRs that ended at #2944

What type of PR is this? (check all applicable)
Related issues
NativeSeriesProtocolΒ #2159TypeVar(s) used in(Data|Lazy)FrameΒ #2356Checklist
If you have comments or can explain your changes, please do so below
Note
Expecting this to cause issues in downstream ci (at least initially)
Everything here (and it is a biggy) stems from this small addition:
The presence of
__len__is enough to avoid overlapping@overloads π€―