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

Add :time/period schema to experimental.time #957

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

dvingo
Copy link
Contributor

@dvingo dvingo commented Sep 20, 2023

Adds :time/period to the experimental time schemas.

@ikitommi
Copy link
Member

Thanks for the PR!

I looked quickly at ISO_8601 and it describes also time-based durations, e.g. syntax of PnYnMnDTnHnMnS.

For example, "P3Y6M4DT12H30M5S" represents a duration of "three years, six months, four days, twelve hours, thirty minutes, and five seconds".

TC39, it's called duration and supports also both dates and times.

In js-joda & java-time, Period and Duration are separate things:

When you write code to specify an amount of time, use the class or method that best meets your needs: the Duration class, Period class, or the ChronoUnit.between method. A Duration measures an amount of time using time-based values (seconds, nanoseconds). A Period uses date-based values (years, months, days).

options:

  1. go just with java-time/js-joda Period, add later separate Duration
  2. go just with java-time/js-joda Period, add later Duration that is the full ISO-thing
  3. go now with :time/duration which supports both Period & Duration

compliancy-wise I would like to see 3, but might be awkward with java-time & js-joda apis?

@dvingo
Copy link
Contributor Author

dvingo commented Sep 24, 2023

Thanks for the review and feedback!

I wasn't aware about ISO_8601 and TC39 combining the two together, I have only been using the java.time (and js-joda) versions in my applications and so that's where the Period/Duration distinction is coming from.

Incidentally, I have felt the need for combining the two together for a while and made my own abstraction called Offset here: https://github.com/dvingo/tick-util/blob/master/src/main/space/matterandvoid/tick_utils/offset.cljc it serializes to a string differently than described by ISO_8601, but that could easily be changed.

I was under the impression that the malli :time/* schemas are intended to follow the semantics of java.time, but it seems like the desire is to have something that matches the standards instead.

Perhaps the Offset idea could be adapted to implement a malli :time/iso-duration (or different name) to avoid conflict with the java.time semantics of the duration name?

@ikitommi
Copy link
Member

ikitommi commented Oct 2, 2023

The original idea was to make something "final" but I don't think it's possible here. e.g. java.time is de facto and I guess TC39 will be too. But they have different concepts.

Current time is modelled based on java.time & js-joda, so I think this PR makes a lot of sense - and the PR is well crafted.

Will get back to this soon.

@ikitommi ikitommi merged commit 8a864a3 into metosin:master Oct 31, 2023
9 checks passed
@ikitommi
Copy link
Member

yes, let's do this. Thanks!!

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.

2 participants