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

humantime serde #811

Merged
merged 21 commits into from
May 14, 2024
Merged

humantime serde #811

merged 21 commits into from
May 14, 2024

Conversation

michaeldjeffrey
Copy link
Contributor

Bring Durations and DateTimes into alignment across settings.
Companion Ops PR: https://github.com/novalabsxyz/ops/pull/1348

A Bit about Duration

When possible std::time::Duration was preferred in place of chrono::Duration.

If there was a duration or timestamp comparison that looked like negative Durations could be in play chrono::Duration was left. (In future chrono versions they've renamed chrono::Duration to chrono::TimeDelta with the thinking that Duration implies only positive time.)

There are also some tests with chrono::Duration is left in because the constructors added readability.
There's a current nightly only feature that will add some of these constructors to std::time::Duration rust-lang/rust#120301

.u(hhhh)nwrap()???

In an attempt to make default duration settings easier to understand, I used humantime::format_duration which returns a result. These are only used in serde default functions.

.poll_duration(settings.reward_check_interval())
.offset(settings.reward_check_interval() * 2)
.lookback(LookbackBehavior::StartAfter(settings.start_after))
.poll_duration(reward_check_interval)
Copy link
Contributor

Choose a reason for hiding this comment

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

The file_info_poller takes a chrono Duration, just to turn it into a std Duration, maybe we should just change it to accept a std Duration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes good sense to me. Also because it doesn't make sense for file info poller to need to support negative time deltas.

@michaeldjeffrey michaeldjeffrey merged commit b040ebc into main May 14, 2024
1 check passed
@michaeldjeffrey michaeldjeffrey deleted the mj/humantime-serde branch May 14, 2024 20:27
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