speedate icon indicating copy to clipboard operation
speedate copied to clipboard

Support explicitly parsing timestamps as seconds/milliseconds

Open ariebovenberg opened this issue 2 years ago • 5 comments

As discussed in https://github.com/pydantic/pydantic/issues/7940, we wish to be able to explicitly specify the unit in which unix timestamsp are to be interpreted.

See comments below for additional context

ariebovenberg avatar Nov 08 '23 20:11 ariebovenberg

Codecov Report

Merging #54 (499c5a7) into main (ce08d32) will increase coverage by 0.12%. The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #54      +/-   ##
==========================================
+ Coverage   99.09%   99.22%   +0.12%     
==========================================
  Files           6        6              
  Lines         885      899      +14     
==========================================
+ Hits          877      892      +15     
+ Misses          8        7       -1     
Files Coverage Δ
src/date.rs 100.00% <100.00%> (ø)
src/datetime.rs 100.00% <100.00%> (ø)
src/lib.rs 100.00% <ø> (+8.33%) :arrow_up:
src/time.rs 97.82% <100.00%> (+0.04%) :arrow_up:

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ce08d32...499c5a7. Read the comment docs.

codecov[bot] avatar Nov 08 '23 20:11 codecov[bot]

The changes so far are a 'naive' implementation of this idea — simply adding the TimestampUnit enum, and adding it as a parameter wherever it is needed. It turns out it would need to be added in quite some places!

Open questions

  • Adding a TimestampUnit argument to so many methods is obviously a big breaking change. One alternative would be a new 'family' of methods called parse_str_with_config or similar — assuming other aspects of parsing could become configurable in future. What are your thoughts on this?
  • On a related note: there was a wrinkle in refactoring the tests, as Duration and Time don't require the TimestampUnit parameter. I solved this using a trait, but I wouldn't be surprised it there is a more elegant way that is obvious to more experienced rustacians.
  • I put the TimestampUnit enum in date.rs since that's where the timestamp watershed constants were already defined. Does it make sense there?

Also TODO still: updating the docs, once API is approved.

ariebovenberg avatar Nov 08 '23 21:11 ariebovenberg

@ariebovenberg maybe I can offer some ideas here (even if @samuelcolvin gets the ultimate decision):

  • regarding TimestampUnit, I think maybe we should just bite the bullet and have DateConfig and DateTimeConfig structures, rather than treat this as an isolated parameter? That'll make it easier to not have breaking changes again in future.

  • potentially the tests also get solved by having parse_str and parse_str_with_config separation, like we have for time?

  • it's fine to have TimestampUnit in date.rs; the public export is speedate::TimestampUnit so we can always move this later without users ever knowing.

davidhewitt avatar Dec 13 '23 18:12 davidhewitt