Skip to content
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

depr(python,rust!): Rename dt.seconds to dt.total_seconds (likewise for days, hours, minutes, milliseconds, microseconds, and nanoseconds) #12179

Merged
merged 5 commits into from
Nov 2, 2023

Conversation

MarcoGorelli
Copy link
Collaborator

@MarcoGorelli MarcoGorelli commented Nov 1, 2023

closes #6445

as mentioned in the issue, .seconds looks like .seconds from pandas / Python stdlib, but it actually equivalent to .total_seconds

… days, hours, minutes, milliseconds, microseconds, and nanoseconds) for temporal types
@github-actions github-actions bot added internal An internal refactor or improvement python Related to Python Polars labels Nov 1, 2023
@stinodego stinodego changed the title refactor(python): rename dt.seconds to dt.total_seconds (likewise for days, hours, minutes, milliseconds, microseconds, and nanoseconds) for temporal types depr(python): rename dt.seconds to dt.total_seconds (likewise for days, hours, minutes, milliseconds, microseconds, and nanoseconds) for temporal types Nov 1, 2023
@github-actions github-actions bot added the deprecation Add a deprecation warning to outdated functionality label Nov 1, 2023
@stinodego stinodego removed the internal An internal refactor or improvement label Nov 1, 2023
@MarcoGorelli MarcoGorelli marked this pull request as ready for review November 1, 2023 20:45
Copy link
Member

@stinodego stinodego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original idea of the linked ticket was to have a single method, i.e. dt.seconds , which would work for both datetime columns (gives you a number between 0 - 59) and for duration columns (gives you the duration in seconds).

However, I think your proposal is better. I like keeping second, hour, etc. for datetime types, and then have total_seconds etc. for duration types. Would be good to get @ritchie46 's signoff here though before we make such changes to the public API.

A few minor comments!

py-polars/docs/source/reference/series/temporal.rst Outdated Show resolved Hide resolved
py-polars/polars/expr/datetime.py Outdated Show resolved Hide resolved
py-polars/polars/series/datetime.py Outdated Show resolved Hide resolved
py-polars/polars/expr/datetime.py Outdated Show resolved Hide resolved
@stinodego stinodego changed the title depr(python): rename dt.seconds to dt.total_seconds (likewise for days, hours, minutes, milliseconds, microseconds, and nanoseconds) for temporal types depr(python,rust!): Rename dt.seconds to dt.total_seconds (likewise for days, hours, minutes, milliseconds, microseconds, and nanoseconds) Nov 2, 2023
@github-actions github-actions bot added breaking rust Change that breaks backwards compatibility for the Rust crate rust Related to Rust Polars labels Nov 2, 2023
@@ -158,7 +158,7 @@ impl PyExpr {
)
.into()
}
fn duration_hours(&self) -> Self {
fn dt_total_hours(&self) -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like these are not on the Rust expression architecture yet - we can add that in a different PR!

Copy link
Member

@stinodego stinodego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has my blessing! But as I said, since it's a change to the public API, we should run this by @ritchie46 as well.

@ritchie46
Copy link
Member

LGTM. 👍

Thanks for the ping @stinodego

@ritchie46 ritchie46 merged commit 8d11987 into pola-rs:main Nov 2, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking rust Change that breaks backwards compatibility for the Rust crate deprecation Add a deprecation warning to outdated functionality python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify temporal expression functions
3 participants