Impl serde::Serialize and serde::Deserialize for TimeDelta

[Awpteamoose]

Could rewrite this to use ISO 8601 representation, which since 2019 allows negative durations, if preferred. However, parsing then is a little ambiguous wrt what to do about years/months.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.12%. Comparing base (081c648) to head (69ad241). Report is 1 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1599 +/- ## ======================================= Coverage 91.11% 91.12% ======================================= Files 37 37 Lines 17104 17123 +19 ======================================= + Hits 15584 15603 +19 Misses 1520 1520 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[JoshLambda]

Related to https://github.com/chronotope/chrono/issues/117

[iddm]

Why has it not been reviewed yet? It is very important and a must-have in 2024.

[djc]

Why has it not been reviewed yet? It is very important and a must-have in 2024.

Because chrono is a gift and you don't get to complain about gifts. Happy to have a conversation about my commercial rates for your must-haves.

[iddm]

Why has it not been reviewed yet? It is very important and a must-have in 2024.

Because chrono is a gift and you don't get to complain about gifts. Happy to have a conversation about my commercial rates for your must-haves.

No need to be offensive, pal. Many of us contribute to open-source projects. I can give you my rates in exchange. Should we speak this language? The meaning of the "must-have" was to emphasize that it has been more than a month for such a simple PR to be reviewed, and I believe it hasn't ever worked for anyone since the breaking change. I wasn't asking you to work overtime or to do something extra, right? Just give it a minute the next time you are going to go through the PRs.

Perhaps I will contribute to this chrono myself; who knows? But then, we still will have to be able to review the pull requests :-)

Thanks for the gift :-)

[djc]

Why has it not been reviewed yet? It is very important and a must-have in 2024.

Because chrono is a gift and you don't get to complain about gifts. Happy to have a conversation about my commercial rates for your must-haves.

No need to be offensive, pal.

The tone of "been reviewed yet", "very important", and "must-have" feel like you're putting on the pressure, and I'm not really a fan of your "pal" there, either. I'm open to reminders that something needs my attention, but as a volunteer maintainer I think it's reasonable you adopt a tone that's more mindful of the volunteer nature of a lot of open source maintenance.

[iddm]

Why has it not been reviewed yet? It is very important and a must-have in 2024.

Because chrono is a gift and you don't get to complain about gifts. Happy to have a conversation about my commercial rates for your must-haves.

No need to be offensive, pal.

The tone of "been reviewed yet", "very important", and "must-have" feel like you're putting on the pressure, and I'm not really a fan of your "pal" there, either. I'm open to reminders that something needs my attention, but as a volunteer maintainer I think it's reasonable you adopt a tone that's more mindful of the volunteer nature of a lot of open source maintenance.

I am really sorry to hear you felt the pressure from my side. Note that perceiving the tone is subjective still, and again, I didn't mean anything bad to offend you or anyone else. I just stated the facts, that's all. The fact that this thing is important, in my opinion, hasn't been reviewed for a while. I was curious to know why cuz perhaps someone might have reviewed it and forgotten to close for some reason, or something has already been done, somewhere out of this GitHub repo and PR discussions, which I wasn't a part of and couldn't know otherwise.

We are all volunteers, and I think even the guy who created this PR was a volunteer, an unpaid guy spending his time on this instead of spending it with his friends and family, playing games or on some other kind of entertainment or "the time for yourself". We all do that. This is GitHub, after all. I am now afraid of saying anything else about this topic, as I am afraid to be perceived as offensive. I just hope you don't feel any pressure anymore and that I meant anything bad, even if the words you saw you perceived otherwise. I don't go from a PR to a PR in random projects to say how bad someone is.

Thanks for taking a look at the PR.

[djc]

I am really sorry to hear you felt the pressure from my side. Note that perceiving the tone is subjective still, and again, I didn't mean anything bad to offend you or anyone else. I just stated the facts, that's all. The fact that this thing is important, in my opinion, hasn't been reviewed for a while.

Appreciate the apology. Agreed that perceiving the tone is subjective, and yet we all should try to be mindful of how our tone was perceived. FWIW, my go to ping is something like "Gentle reminder" or "Gentle ping", which I think has enough of an effect in most cases.

I was curious to know why cuz perhaps someone might have reviewed it and forgotten to close for some reason, or something has already been done something out of this GitHub repo and PR discussions, which I wasn't a part of and couldn't know otherwise.

I think it was still in my notification queue, but snowed under by other review requests that I deemed more important.

I am now afraid of saying anything else about this topic, as I am afraid to be perceived as offensive.

No need to be afraid, thanks for the candor.

If @Awpteamoose doesn't come back to address the comments in a few days, maybe you want to take these changes and resubmit the PR? I'd be happy to rereview and publish a release once it's merged.

[iddm]

I am really sorry to hear you felt the pressure from my side. Note that perceiving the tone is subjective still, and again, I didn't mean anything bad to offend you or anyone else. I just stated the facts, that's all. The fact that this thing is important, in my opinion, hasn't been reviewed for a while.

Appreciate the apology. Agreed that perceiving the tone is subjective, and yet we all should try to be mindful of how our tone was perceived. FWIW, my go to ping is something like "Gentle reminder" or "Gentle ping", which I think has enough of an effect in most cases.

I was curious to know why cuz perhaps someone might have reviewed it and forgotten to close for some reason, or something has already been done something out of this GitHub repo and PR discussions, which I wasn't a part of and couldn't know otherwise.

I think it was still in my notification queue, but snowed under by other review requests that I deemed more important.

I am now afraid of saying anything else about this topic, as I am afraid to be perceived as offensive.

No need to be afraid, thanks for the candor.

If @Awpteamoose doesn't come back to address the comments in a few days, maybe you want to take these changes and resubmit the PR? I'd be happy to rereview and publish a release once it's merged.

Thanks, I will borrow your ping messages :-)

Yes, I will gladly make a PR. I would love to use the OOTB parser, as for now, I specify the TimeDelta in my toml as a dictionary, and it works well (I haven't tested it, but at least it is parsed to something):

server_allowed_unused_time = { secs = 600, nanos = 0 }

This may help someone else work around this issue, so I am sharing it here.

[BurntSushi]

Has using the ISO 8601 duration format been considered?

[iddm]

Has using the ISO 8601 duration format been considered?

It would have been awesome to have it specified as ISO 8601. Actually, this was precisely the first thing I tried to no avail. @djc do you have any objections?

[Awpteamoose]

I'm around, I'm AFK today but I'll address the comments tomorrow.

Has using the ISO 8601 duration format been considered?

If there's consensus on what to do about relative units (months, years, even days) when parsing - I can use that. My opinion would be to introduce RelativeTimeDelta and have that use ISO 8601 with full support and keep TimeDelta as-is. I do have a usecase for RelativeTimeDelta so I'm interested in contributing that as well, but separately from this PR.

[BurntSushi]

You could return an error for unsupported units.

[djc]

We have #1290. It feels to me like using ISO 8601 duration syntax for the value of TimeDelta would cause an impedance mismatch because TimeDelta cannot accurately be deserialized from many ISO 8601 duration values. We could yield an error, but I'd prefer doing something that has better type-safety at compile time.

[BurntSushi]

The benefit of using ISO 8601 is that you improve interoperability with other systems. TimeDelta cannot support all ISO 8601 durations, but ISO 8601 can support all TimeDelta values. By using ISO 8601, other systems should be able to more easily deserialize it.

[djc]

The benefit of using ISO 8601 is that you improve interoperability with other systems. TimeDelta cannot support all ISO 8601 durations, but ISO 8601 can support all TimeDelta values. By using ISO 8601, other systems should be able to more easily deserialize it.

Any implementation of Deserialize for TimeDelta against ISO 8601 durations would be incomplete, which IMO is a footgun that will lead to support issues I'd rather avoid.

[BurntSushi]

Any implementation of Deserialize for TimeDelta against ISO 8601 durations would be incomplete, which IMO is a footgun that will lead to support issues I'd rather avoid.

Note that java.time's Duration type supports parsing the ISO 8601 format. It specifically rejects durations with units of weeks or higher. See https://github.com/openjdk/jdk/blob/0fdfdf71f242b39f2e758fcff99bd61060fa2870/src/java.base/share/classes/java/time/Duration.java#L390-L420 and https://github.com/openjdk/jdk/blob/0fdfdf71f242b39f2e758fcff99bd61060fa2870/src/java.base/share/classes/java/time/Duration.java#L153-L158.

I'm not plugged into the Java world enough to know whether this has created a footgun that spurs support issues.

Python's timedelta doesn't define a serialization format at all. NodaTime's Duration doesn't seam tosupport ISO 8601 (I think, it's a little hard to tell from its docs), but its Period type definitely does.

[djc]

@pitdicker would you have a chance to have a look at this?

[djc]

@pitdicker friendly ping to have a look, please.

[djc]

Thanks!