-
Notifications
You must be signed in to change notification settings - Fork 124
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
chore: IntoImplementation
type alias
#1917
base: main
Are you sure you want to change the base?
Conversation
thanks for doing this not totally sure about this one, some thoughts:
|
Makes sense! I am ok with leaving the signature as it is with the union
This would require to rethink the codebase, not just the signature π |
https://narwhals-dev.github.io/narwhals/api-reference/dataframe/#narwhals.dataframe.DataFrame.lazy
@MarcoGorelli I imagined the docs for DataFrame.lazy would work in the same way as from_native
Is that not a positive? |
I thought it would, but when I built this locally then it didn't show up. Though for some reason |
Oh, is that because there isn't a page for I went looking for one when I wrote this but settled on linking to source: |
right, thanks π€¦ maybe we should make a type alias for |
IntoImplementation: TypeAlias = Union["Implementation", ModuleType, str] | ||
"""Anything which can be converted to a Narwhals Implementation via Implementation.from_backend.""" | ||
|
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, is that because there isn't a page for Implementation?
right, thanks π€¦
maybe we should make a type alias for
Backend
, rather thanIntoImplementation
?
@MarcoGorelli responding here as it has the context.
I did originally propose either IntoBackend
or IntoImplementation
in (#1914 (comment))
Although (IntoImplementation
) is a mouthful, I do think its the best description of what is acceptable.
IntoBackend
would make more sense if the enum were renamed.
I think having Implementation
and Backend
could be a point of confusion - since they're semantically similar but mean very different things to narwhals
Note
I'd personally leave the description as:
Anything which can be converted to a Narwhals Implementation.
What type of PR is this? (check all applicable)
Related issues
Checklist
If you have comments or can explain your changes, please do so below