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

Support explicitly parsing timestamps as seconds/milliseconds #54

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ariebovenberg
Copy link

@ariebovenberg ariebovenberg commented Nov 8, 2023

As discussed in pydantic/pydantic#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

Copy link

codecov bot commented Nov 8, 2023

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%) ⬆️
src/time.rs 97.82% <100.00%> (+0.04%) ⬆️

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.

@ariebovenberg ariebovenberg force-pushed the timestamp-unit branch 2 times, most recently from 22229de to d0b32b0 Compare November 8, 2023 21:05
@ariebovenberg ariebovenberg marked this pull request as ready for review November 8, 2023 21:12
@ariebovenberg
Copy link
Author

ariebovenberg commented Nov 8, 2023

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.

@davidhewitt
Copy link
Contributor

@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.

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.

None yet

2 participants