-
Notifications
You must be signed in to change notification settings - Fork 125
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
fix: Use metaclass for safe DType
attr access
#2025
Conversation
Both `Duration` and `Datetime` are working with `polars` now. From this point it should just be reducing code for all the other backends
They're all noops, but good to keep consistent
return pl.Duration(time_unit=du_time_unit) # type: ignore[arg-type] | ||
if isinstance_or_issubclass(dtype, dtypes.Datetime): | ||
# [s] is not allowed for polars | ||
return pl.Datetime(dtype.time_unit, dtype.time_zone) # type: ignore[arg-type] |
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.
time_unit="s"
isn't allowed for polars
, but we pass it through anyway?
The metaclass is much narrower than `type` previously
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.
I didn't think i liked metaclasses, but this is pretty neat - thanks @dangotbanned !
Yeah I know - they can be complex some times - but this is probably the simplest use case for them π Really wasn't expecting everything to go so smoothly |
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 @dangotbanned !
DType
attr accessDType
attr access
What type of PR is this? (check all applicable)
Related issues
Mentioned in
Checklist
If you have comments or can explain your changes, please do so below
Datetime
, when used in_polars
onlyThis gives you a read-only default for:
Which is unaffected by the instance-level version:
I imagined this would also be useful
Duration
:narwhals/narwhals/dtypes.py
Line 527 in 43d072b
Originally thought this would be beneficial for the
NestedType
classes, but I'm seeing that they don't have defaults