pyo3 icon indicating copy to clipboard operation
pyo3 copied to clipboard

Add chrono support

Open pickfire opened this issue 3 years ago • 2 comments

Moved from https://github.com/chronotope/chrono/pull/542

What's left

  • [x] port tests
  • [ ] wait for chrono upstream to accept the changes https://github.com/chronotope/chrono/pull/812

cc @Psykopear I just pushed so that you can help review even though it's not complete yet, but all the types are now there

pickfire avatar Aug 31 '22 15:08 pickfire

That's great, thank you! A couple of things:

  • You mention the need for an update on the time crate, to get the SECS_PER_DAY const, but the const is defined inside chrono itself (in src/time_delta.rs), which doesn't depend on time anymore. It's not public, but we can either just change that, or define it here, it's a fixed number and I hope it doesn't change (not being ironic, I just think I can't assume anything when talking about datetimes)
  • What you do here for the conversion can be done with the current chrono api, it's less efficient (using date.year() ecc., rather than exposing date.mdf()), but it would work without any change to chrono. It might make sense to make this PR work with the current version of chrono, and maybe update it once chrono exposes the needed api? I say this because I feel like the changes on chrono would not be trivial, they'd need to expose some structs/functions that are currently private, and this might be a long process. Also, it's should be an implementation detail here, updating it later won't change anything in the exposed functions/structs.

edit: I'll make a proper review of the code tomorrow morning, I'll also try it on the library I'm working on at the moment, where I have some tests for the python integration, and let you know if I have any problem

Psykopear avatar Aug 31 '22 15:08 Psykopear

@pickfire I opened a PR https://github.com/pickfire/pyo3/pull/1 over your fork, I updated this to work with the published version of chrono, without any change to the api. Let me know what you think, and if you like the idea feel free to merge it.

Psykopear avatar Sep 01 '22 14:09 Psykopear