-
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
patch: Improving typing of stable from_native
when 'strict=False'
#875
patch: Improving typing of stable from_native
when 'strict=False'
#875
Conversation
from_native
when 'srict=False'from_native
when 'strict=False'
thanks @luke396 ! do we need to make these changes in |
Thanks for the reminder! I will handle it and push it later. |
tests/stable_api_test.py
Outdated
def test_from_native_strict_false_typing() -> None: | ||
df = pl.DataFrame() | ||
nw_v1.from_native(df, strict=False) | ||
nw_v1.from_native(df, strict=False, eager_only=True) | ||
nw_v1.from_native(df, strict=False, eager_or_interchange_only=True) |
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.
in tests/translate/from_native_test.py
we're already doing import narwhals.stable.v1 as nw
how about, in tests/translate/from_native_test.py
, to add a test which duplicates test_from_native_strict_false_typing
but which uses unstable_nw
instead of nw
?
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.
thanks @luke396 !
def test_from_native_strict_false_typing() -> None: | ||
df = pl.DataFrame() | ||
nw.from_native(df, strict=False) | ||
nw.from_native(df, strict=False, eager_only=True) | ||
nw.from_native(df, strict=False, eager_or_interchange_only=True) | ||
|
||
unstable_nw.from_native(df, strict=False) | ||
unstable_nw.from_native(df, strict=False, eager_only=True) | ||
unstable_nw.from_native(df, strict=False, eager_or_interchange_only=True) |
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.
question out of curiosity: it's not super clear to me what we are testing here. wouldn't mypy
be able to catch errors for typing? π
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.
yeah it's just to check that mypy doesn't complain on these lines
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.
In our previous codebase, there was no such combination of parameters present, neither in 'narwhals' nor in 'tests'.
Thus, I believe this is the reason why the typing issue is resolved now, rather than before.
What type of PR is this? (check all applicable)
Related issues
from_native
whenstrict=False
Β #833Checklist
If you have comments or can explain your changes, please do so below.
@EdAbati helps a lot in discord! Thanks again!