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

Awpteamoose · 2024-07-17T14:05:56Z

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 · 2024-07-17T14:14:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JoshLambda · 2024-07-26T11:05:46Z

Related to #117

iddm · 2024-09-03T13:10:25Z

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

djc · 2024-09-04T07:46:44Z

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.

djc · 2024-09-04T07:48:16Z

src/time_delta.rs

@@ -62,6 +62,28 @@ pub struct TimeDelta {
    nanos: i32, // Always 0 <= nanos < NANOS_PER_SEC
 }

+#[cfg(feature = "serde")]
+impl serde::Serialize for TimeDelta {


Nit: I'd prefer to have the serde import at the top of the file (with a cfg guard if need be), to keep this code a little clear/easier to read.

What's noisy here? serde::Serialize? Would you prefer just impl Serialize here? Also, if we import the namespace of serde at the top, we will have one additional #[cfg(feature = "serde")], and this one won't go.
I think, though, the implementations might be under a mod which has just one #[cfg(feature = "serde")] and all the imports there and all the tests. Would it work?

Yes, the serde:: prefixes throughout these impls. I'm less worried about the cfg guards, but agreed that an inline mod might be nicer here.

Thanks for the clarification.

I will try to do it later today if the original author won't be able to do it by that time. But this will be a separate PR, as I can't push to the author's branch.

iddm · 2024-09-04T07:56:34Z

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 · 2024-09-04T07:59:45Z

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 · 2024-09-04T08:06:45Z

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 · 2024-09-04T08:12:22Z

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 · 2024-09-04T08:24:34Z

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 · 2024-09-04T11:09:34Z

Has using the ISO 8601 duration format been considered?

iddm · 2024-09-04T11:28:34Z

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 · 2024-09-04T14:08:52Z

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 · 2024-09-04T14:12:31Z

You could return an error for unsupported units.

djc · 2024-09-05T08:02:57Z

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 · 2024-09-05T11:35:21Z

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 · 2024-09-05T11:41:05Z

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 · 2024-09-05T14:33:59Z

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 · 2024-09-09T08:05:49Z

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

djc · 2024-09-16T08:45:02Z

@pitdicker friendly ping to have a look, please.

pitdicker

This serialization makes sense to me. I also prefer to keep a full ISO 8601 format for a more complete type and not use it for TimeDelta's.

src/time_delta.rs

djc · 2024-09-16T10:10:37Z

Thanks!

pitdicker · 2024-09-16T10:23:25Z

src/time_delta.rs

-                return Err(Error::custom("TimeDelta out of bounds"));
-            }
-            Ok(TimeDelta { secs, nanos })
+            TimeDelta::new(secs, nanos as u32).ok_or(Error::custom("TimeDelta out of bounds"))


Sorry, comment after the merge. What happens if nanos is negative?

Edit: never mind, it works out.

Awpteamoose temporarily deployed to Coverage July 17, 2024 14:08 — with GitHub Actions Inactive

Awpteamoose temporarily deployed to Coverage July 17, 2024 14:11 — with GitHub Actions Inactive

Awpteamoose force-pushed the serde-timedelta branch from 315649e to dfe5349 Compare July 17, 2024 14:16

Awpteamoose temporarily deployed to Coverage July 17, 2024 14:16 — with GitHub Actions Inactive

Awpteamoose temporarily deployed to Coverage July 17, 2024 14:20 — with GitHub Actions Inactive

djc approved these changes Sep 4, 2024

View reviewed changes

Awpteamoose temporarily deployed to Coverage September 6, 2024 10:42 — with GitHub Actions Inactive

Awpteamoose force-pushed the serde-timedelta branch from b784082 to 8dd3795 Compare September 6, 2024 10:42

Awpteamoose temporarily deployed to Coverage September 6, 2024 10:44 — with GitHub Actions Inactive

Awpteamoose requested a review from djc September 6, 2024 10:45

Awpteamoose added 2 commits September 12, 2024 10:52

impl serde for timedelta

60bf7b7

fmt

6451c9f

Awpteamoose added 2 commits September 12, 2024 10:52

add a panic test

35c1ac5

inline mod

3ac01e3

Awpteamoose force-pushed the serde-timedelta branch from 8dd3795 to 3ac01e3 Compare September 12, 2024 09:52

Awpteamoose temporarily deployed to Coverage September 12, 2024 09:53 — with GitHub Actions Inactive

pitdicker reviewed Sep 16, 2024

View reviewed changes

src/time_delta.rs Outdated Show resolved Hide resolved

use TimeDelta::new instead of repeating checks

3c83d58

Awpteamoose temporarily deployed to Coverage September 16, 2024 09:56 — with GitHub Actions Inactive

fmt

69ad241

Awpteamoose temporarily deployed to Coverage September 16, 2024 09:58 — with GitHub Actions Inactive

Awpteamoose requested a review from pitdicker September 16, 2024 09:59

djc merged commit c8defc7 into chronotope:main Sep 16, 2024
35 checks passed

pitdicker reviewed Sep 16, 2024

View reviewed changes

Awpteamoose deleted the serde-timedelta branch September 16, 2024 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Awpteamoose commented Jul 17, 2024 •

edited

Loading

codecov bot commented Jul 17, 2024 •

edited

Loading

JoshLambda commented Jul 26, 2024

iddm commented Sep 3, 2024

djc commented Sep 4, 2024

djc Sep 4, 2024

iddm Sep 4, 2024 •

edited

Loading

djc Sep 4, 2024

iddm Sep 4, 2024

iddm commented Sep 4, 2024

djc commented Sep 4, 2024 •

edited

Loading

iddm commented Sep 4, 2024 •

edited

Loading

djc commented Sep 4, 2024

iddm commented Sep 4, 2024

BurntSushi commented Sep 4, 2024

iddm commented Sep 4, 2024

Awpteamoose commented Sep 4, 2024 •

edited

Loading

BurntSushi commented Sep 4, 2024

djc commented Sep 5, 2024 •

edited

Loading

BurntSushi commented Sep 5, 2024

djc commented Sep 5, 2024

BurntSushi commented Sep 5, 2024

djc commented Sep 9, 2024

djc commented Sep 16, 2024

pitdicker left a comment

djc commented Sep 16, 2024

pitdicker Sep 16, 2024 •

edited

Loading

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

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

Conversation

Awpteamoose commented Jul 17, 2024 • edited Loading

codecov bot commented Jul 17, 2024 • edited Loading

Codecov Report

JoshLambda commented Jul 26, 2024

iddm commented Sep 3, 2024

djc commented Sep 4, 2024

djc Sep 4, 2024

Choose a reason for hiding this comment

iddm Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

djc Sep 4, 2024

Choose a reason for hiding this comment

iddm Sep 4, 2024

Choose a reason for hiding this comment

iddm commented Sep 4, 2024

djc commented Sep 4, 2024 • edited Loading

iddm commented Sep 4, 2024 • edited Loading

djc commented Sep 4, 2024

iddm commented Sep 4, 2024

BurntSushi commented Sep 4, 2024

iddm commented Sep 4, 2024

Awpteamoose commented Sep 4, 2024 • edited Loading

BurntSushi commented Sep 4, 2024

djc commented Sep 5, 2024 • edited Loading

BurntSushi commented Sep 5, 2024

djc commented Sep 5, 2024

BurntSushi commented Sep 5, 2024

djc commented Sep 9, 2024

djc commented Sep 16, 2024

pitdicker left a comment

Choose a reason for hiding this comment

djc commented Sep 16, 2024

pitdicker Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Awpteamoose commented Jul 17, 2024 •

edited

Loading

codecov bot commented Jul 17, 2024 •

edited

Loading

iddm Sep 4, 2024 •

edited

Loading

djc commented Sep 4, 2024 •

edited

Loading

iddm commented Sep 4, 2024 •

edited

Loading

Awpteamoose commented Sep 4, 2024 •

edited

Loading

djc commented Sep 5, 2024 •

edited

Loading

pitdicker Sep 16, 2024 •

edited

Loading