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

Adds std::duration::Duration from/to Python conversions #3670

Merged
merged 2 commits into from
Dec 20, 2023

Conversation

Tpt
Copy link
Contributor

@Tpt Tpt commented Dec 19, 2023

No description provided.

@Tpt Tpt changed the title Adds from/to Python conversions to std::duration::Duration Adds std::duration::Duration from/to Pytho conversions Dec 19, 2023
@Tpt Tpt changed the title Adds std::duration::Duration from/to Pytho conversions Adds std::duration::Duration from/to Python conversions Dec 19, 2023
@Tpt
Copy link
Contributor Author

Tpt commented Dec 19, 2023

We already support chrono::Duration, it might be nice to support std::duration::Duration for consistency.

Things to notice:

  • std::duration::Duration only supports positive durations, an exception is raised when converting a negative timedelta
  • a warning is created if precision is lost when converting from Duration to timedelta, opposite to chrono::Duration. Is it a good idea?

Copy link
Member

@adamreichold adamreichold left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

👍 likewise, thanks!

It's a little sad that Rust stdlib duration is strictly positive, but that's not really our design problem to worry about here.

@davidhewitt
Copy link
Member

Hmm, there are some test failures?

  • a warning is created if precision is lost when converting from Duration to timedelta, opposite to chrono::Duration. Is it a good idea?

Yeah I think that's fine to warn 👍

@Tpt
Copy link
Contributor Author

Tpt commented Dec 19, 2023

Thank you for the review!

Hmm, there are some test failures?

My bad. It should be fixed now.

a warning is created if precision is lost when converting from Duration to timedelta, opposite to chrono::Duration. Is it a good idea?

Yeah I think that's fine to warn 👍

After thinking a bit more about it, I believe warning is a bad idea: it might overflow the warning logs if the conversion is done a lot of times. And it is likely there are cases where the user does not care about the precision loss (for example if the duration is an elapsed time between two timestamp). I would tend to just remove it (like chrono::Duration behaves) or introduce a static boolean to print it only once per process execution. I have removed the warning in this MR but I am happy to add it back if you feel it's useful.

@adamreichold adamreichold added this pull request to the merge queue Dec 19, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 19, 2023
@adamreichold
Copy link
Member

@Tpt Argh, unused import on wasm32-wasi derailed the CI...

@adamreichold
Copy link
Member

Pushed a fixup.

@adamreichold adamreichold added this pull request to the merge queue Dec 20, 2023
Merged via the queue into PyO3:main with commit 3583b9a Dec 20, 2023
37 checks passed
@Tpt Tpt deleted the duration branch December 21, 2023 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants