metosin / malli

High-performance data-driven data specification library for Clojure/Script.
Eclipse Public License 2.0
1.47k stars 209 forks source link

Add initial implementation for java time types schemas #802

Closed bsless closed 1 year ago

bsless commented 1 year ago

Please find below an initial implementation for time schemas

Status: mature draft, at testing phase Scope: JVM, schemas, transformers, generators, JSON schema, swagger

Design:

At various points users can provide a DateTimeFormatter or pattern which will be compiled to one.

bsless commented 1 year ago

@ikitommi I believe the MR is ready for review. I tried to cover everything I could think of, lmk if there are some gaps

bsless commented 1 year ago

Regarding the split and the addition of a bunch of namespaces, we have a few options, which I'm not a big fan of:

Serialization:

timex: No problem, will clean up a bit then make the change. Edit: question, change the ns fragment to timex too, or just the keyword namespace?

Thoughts?

bsless commented 1 year ago

I must say that after s/:time\//:timex\//g it just looks silly. It's just not aesthetic

ikitommi commented 1 year ago
ikitommi commented 1 year ago

serialization, I was thinking just about values, but the schema serialization is another issue. When there is JS/CLJS supported version, support for data + parser would be good. I think we can start with the current way you proposed it.

bsless commented 1 year ago

regarding timex, my suggestion is to just use time

Rationale - these map to the ISO-8601 formatters: https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatter.html

If anything, the prefix could be iso or iso-time. Barring that, just time seems most correct. It's a universal standard we're trying to satisfy

ikitommi commented 1 year ago

I agree :time is correct in the long run. My idea was just that we could use something else in case the schemas change (types, names, properties, formats) before CLJS/JS impl is done. Otherwise, there can be a silent breakage causing hard-to-debug problems if someone has persisted the schemas into db. With new schema name, one needs to migrate the schemas.

That said, current Temporal model doesn't seem that different:

temporal-object-model

I'm merging this now, we can post-tune this before release.

Big thanks for the PR!!

bsless commented 1 year ago

Awesome! I'll prepare a docs update

ikitommi commented 1 year ago

About the easy includes. Maybe something like:

(ns malli.experimental.time.module
  (:require [malli.experimental.time]
            [malli.experimental.time.generator]
            [malli.experimental.time.json-schema]
            [malli.experimental.time.transform]))

(defn schemas []
  (malli.experimental.time/-time-schemas))
ikitommi commented 1 year ago

... IMO sucks, but maybe less than the alternatives.

bsless commented 1 year ago

What about moving it to conditional read as part of malli.core?

ikitommi commented 1 year ago

that would make this non-experimental. We can do that later.

bsless commented 1 year ago

Would have wanted something like Emacs's advice or eval-after-load mechanism

bsless commented 1 year ago

Can we put it in core and gate its evaluation behind a jvm property or dynamic variable?, sort of eval-when?

ikitommi commented 1 year ago

Let's not do that yet. We can merge the time-schemas into the core namespaces when we drop the experimental ns part after proper test-run in projects & design verification on CLJS side. Most (if not all) the projects I've done (and seen) with malli have a custom registry already so it's easy to mount in the new time schemas.

btw, I'm still on holiday this week and mostly not reading emails, so lag in responding.