humantime serde #811
Merged
humantime serde #811
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bbalser
reviewed
May 14, 2024
| .poll_duration(settings.reward_check_interval()) | ||
| .offset(settings.reward_check_interval() * 2) | ||
| .lookback(LookbackBehavior::StartAfter(settings.start_after)) | ||
| .poll_duration(reward_check_interval) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
That makes good sense to me. Also because it doesn't make sense for file info poller to need to support negative time deltas.
bbalser
approved these changes
May 14, 2024
michaeldjeffrey
force-pushed
the
mj/humantime-serde
branch
from
a293d8b
to
3d416e9
Compare
maplant
approved these changes
May 14, 2024
prefer std::time::Duration when possible
std::Durations were being turned into chrono::Durations only to be turned right back into std::Durations. It also doesn't make much sense for the file poller to support negative time deltas, so we can remove that possibility.
michaeldjeffrey
force-pushed
the
mj/humantime-serde
branch
from
3d416e9
to
47f2f65
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bring Durations and DateTimes into alignment across settings.
Companion Ops PR: https://github.com/novalabsxyz/ops/pull/1348
A Bit about
DurationWhen possible
std::time::Durationwas preferred in place ofchrono::Duration.If there was a duration or timestamp comparison that looked like negative Durations could be in play
chrono::Durationwas left. (In future chrono versions they've renamedchrono::Durationtochrono::TimeDeltawith the thinking thatDurationimplies only positive time.)There are also some tests with
chrono::Durationis left in because the constructors added readability.There's a current nightly only feature that will add some of these constructors to
std::time::Durationrust-lang/rust#120301.u(hhhh)nwrap()???In an attempt to make default duration settings easier to understand, I used
humantime::format_durationwhich returns a result. These are only used in serde default functions.