refactor(typing): Simplify concat signature
#2339
Conversation
- Using a constrained `TypeVar`, like in `polars` - Discovered in #2337
Indicates the new signature is working
| TypeError: The items to concatenate should either all be eager, or all lazy | ||
| """ | ||
| return _stableify(nw.concat(items, how=how)) | ||
| return cast("FrameT", _stableify(nw.concat(items, how=how))) |
There was a problem hiding this comment.
Not super happy about using cast - but it'll do for now
|
nice! i was taking a look at concat too, and noticed that we should be disallowing horizontal concatenation for lazyframes in light of that, is the simplification here still valid? |
Thanks @MarcoGorelli EditMoved comment to #2340 (comment) |
Alright - thought about it - yeah this would still be what we'd base the I imagine it would be something like: FrameT = TypeVar("FrameT", "DataFrame[Any]", "LazyFrame[Any]")
DataFrameT = TypeVar("DataFrameT", bound="DataFrame[Any]")
@overload
def concat(
items: Iterable[FrameT], *, how: Literal["vertical", "diagonal"] = ...
) -> FrameT: ...
@overload
def concat(items: Iterable[DataFrameT], *, how: Literal["horizontal"]) -> DataFrameT: ...
def concat(
items: Iterable[FrameT],
*,
how: Literal["horizontal", "vertical", "diagonal"] = "vertical",
) -> FrameT:The actual one might need to account for more default/non-default combinations. |
| TypeError, | ||
| match=("The items to concatenate should either all be eager, or all lazy"), | ||
| ): | ||
| nw.concat([nw.from_native(df, eager_only=True), nw.from_native(df).lazy()]) | ||
| nw.concat([nw.from_native(df, eager_only=True), nw.from_native(df).lazy()]) # type: ignore[type-var] |
There was a problem hiding this comment.
@MarcoGorelli I have no idea if that made sense 😅
I'm trying to say - if we merge (#2341) then I still want to base the new signatures on the new TypeVar.
We currently raise here, but the @overloads pass as valid
What type of PR is this? (check all applicable)
Related issues
polars._typingaliases #2337*(Namespace|DataFrame).from_numpy#2283 (comment)Checklist
If you have comments or can explain your changes, please do so below
TypeVar, like inpolars