-
Notifications
You must be signed in to change notification settings - Fork 188
tie IntoFrameT to NativeFrame
#3496
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: main
Are you sure you want to change the base?
Changes from 18 commits
a11807a
108c8c0
97999f3
9d85f33
890fb17
649077a
6bc4dc1
272bba0
a0cd526
988e533
8706378
4e330a8
d513368
273960e
e6f3bb5
0fa4ce1
3a07d2c
22520ac
ca8e0ac
cc089ed
30cadf4
98d3e44
287be0a
78516e1
6c20fc4
44add0d
a32fb68
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 | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -55,6 +55,7 @@ | |||||||||
| Frame, | ||||||||||
| IntoDataFrameT, | ||||||||||
| IntoFrame, | ||||||||||
| IntoFrameT, | ||||||||||
| IntoLazyFrameT, | ||||||||||
| IntoSeries, | ||||||||||
| IntoSeriesT, | ||||||||||
|
|
@@ -70,12 +71,12 @@ | |||||||||
|
|
||||||||||
| @overload | ||||||||||
| def to_native( | ||||||||||
| narwhals_object: DataFrame[IntoDataFrameT], *, pass_through: Literal[False] = ... | ||||||||||
| ) -> IntoDataFrameT: ... | ||||||||||
| narwhals_object: DataFrame[IntoFrameT], *, pass_through: Literal[False] = ... | ||||||||||
| ) -> IntoFrameT: ... | ||||||||||
| @overload | ||||||||||
| def to_native( | ||||||||||
| narwhals_object: LazyFrame[IntoLazyFrameT], *, pass_through: Literal[False] = ... | ||||||||||
| ) -> IntoLazyFrameT: ... | ||||||||||
| narwhals_object: LazyFrame[IntoFrameT], *, pass_through: Literal[False] = ... | ||||||||||
| ) -> IntoFrameT: ... | ||||||||||
| @overload | ||||||||||
| def to_native( | ||||||||||
| narwhals_object: Series[IntoSeriesT], *, pass_through: Literal[False] = ... | ||||||||||
|
|
@@ -85,12 +86,10 @@ def to_native(narwhals_object: Any, *, pass_through: bool) -> Any: ... | |||||||||
|
|
||||||||||
|
|
||||||||||
| def to_native( | ||||||||||
| narwhals_object: DataFrame[IntoDataFrameT] | ||||||||||
| | LazyFrame[IntoLazyFrameT] | ||||||||||
| | Series[IntoSeriesT], | ||||||||||
| narwhals_object: DataFrame[IntoFrameT] | LazyFrame[IntoFrameT] | Series[IntoSeriesT], | ||||||||||
| *, | ||||||||||
| pass_through: bool = False, | ||||||||||
| ) -> IntoDataFrameT | IntoLazyFrameT | IntoSeriesT | Any: | ||||||||||
| ) -> IntoFrameT | IntoSeriesT | Any: | ||||||||||
| """Convert Narwhals object to native one. | ||||||||||
|
|
||||||||||
| Arguments: | ||||||||||
|
|
@@ -129,6 +128,10 @@ def from_native( | |||||||||
| @overload | ||||||||||
| def from_native(native_object: LazyFrameT, **kwds: Unpack[AllowLazy]) -> LazyFrameT: ... | ||||||||||
| @overload | ||||||||||
| def from_native( | ||||||||||
| native_object: LazyFrameT | DataFrameT, **kwds: Unpack[AllowLazy] | ||||||||||
| ) -> LazyFrameT | DataFrameT: ... | ||||||||||
| @overload | ||||||||||
|
Comment on lines
+131
to
+134
Member
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. If this is now needed, does it mean that this is now a valid annotation?
Suggested change
Member
Author
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. without it, we'd get that |
||||||||||
| def from_native( | ||||||||||
| native_object: IntoDataFrameT, **kwds: Unpack[ExcludeSeries] | ||||||||||
| ) -> DataFrame[IntoDataFrameT]: ... | ||||||||||
|
|
@@ -145,10 +148,18 @@ def from_native( | |||||||||
| native_object: IntoLazyFrameT, **kwds: Unpack[AllowLazy] | ||||||||||
| ) -> LazyFrame[IntoLazyFrameT]: ... | ||||||||||
| @overload | ||||||||||
| def from_native( | ||||||||||
| def from_native( # type: ignore[overload-overlap] | ||||||||||
|
Member
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. ?
Member
Author
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. π€· pyright's fine with it but mypy outputs
Member
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. I'm not feeling convinced by this or (#3496 (comment)) If you remove all the ignore comments, does |
||||||||||
| native_object: IntoDataFrameT | IntoSeriesT, **kwds: Unpack[AllowSeries] | ||||||||||
| ) -> DataFrame[IntoDataFrameT] | Series[IntoSeriesT]: ... | ||||||||||
| @overload | ||||||||||
| def from_native( | ||||||||||
| native_object: IntoDataFrameT | IntoLazyFrameT, **kwds: Unpack[AllowLazy] | ||||||||||
| ) -> DataFrame[IntoDataFrameT] | LazyFrame[IntoLazyFrameT]: ... | ||||||||||
| @overload | ||||||||||
| def from_native( # type: ignore[overload-overlap] | ||||||||||
| native_object: IntoFrameT, **kwds: Unpack[AllowLazy] | ||||||||||
| ) -> DataFrame[IntoFrameT] | LazyFrame[IntoFrameT]: ... | ||||||||||
|
Comment on lines
+158
to
+161
Member
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. I don't know for sure if it'll work, but one way to make the overload defs non-overlapping might be: @overload
def from_native(native_object: IntoFrameT) -> DataFrame[IntoFrameT] | LazyFrame[IntoFrameT]: ...Since |
||||||||||
| @overload | ||||||||||
| def from_native( | ||||||||||
| native_object: IntoDataFrameT | IntoLazyFrameT | IntoSeriesT, **kwds: Unpack[AllowAny] | ||||||||||
| ) -> DataFrame[IntoDataFrameT] | LazyFrame[IntoLazyFrameT] | Series[IntoSeriesT]: ... | ||||||||||
|
|
@@ -164,7 +175,7 @@ def from_native( | |||||||||
| series_only: bool, | ||||||||||
| allow_series: bool | None, | ||||||||||
| ) -> Any: ... | ||||||||||
| def from_native( | ||||||||||
| def from_native( # pyright: ignore[reportInconsistentOverload] | ||||||||||
|
Member
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. ?
Member
Author
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. pyright gives given that this part isn't user-facing (the overloads are used) i didn't it was too big of a deal
Member
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.
How each type checker deals will overlapping overloads is a bit hairy.
AFAICT, there isn't a spec for overlapping definitions π’ This errorBoth
Important The user facing problems I've experienced is that:
I left a screen capture in (zen-xu/pyarrow-stubs#208 (comment)) with an extreme example or how slow it can get π |
||||||||||
| native_object: IntoLazyFrameT | ||||||||||
| | IntoDataFrameT | ||||||||||
| | IntoSeriesT | ||||||||||
|
|
||||||||||
Uh oh!
There was an error while loading. Please reload this page.