Clojurians Log v2

I'd recommend encoding the logic directly in Clojure rather than expanding :if to another schema. https://github.com/frenchy64/malli/blob/84fe04ef21c292226accf2d3c0c791ad450f29f1/src/malli/core.cljc#L832-L969 I can reopen/update the PR @joel.kaasinen mentioned, it just fell off my radar. https://github.com/metosin/malli/pull/1161

I'll try to find time to review those, thanks

Had a look at the comments. The biggest flaws as pointed are actually the parts I understand the least about malli, so answer to those is pretty much "because this is how I got it working". For example I have no idea what "`:if` is not a leaf" actually means and how to fix the issue. So uh, yeah. My abilities are probably exhausted at this point, I can do the point changes for typo fixes and such but for bigger changes I definitely need more documentation, guidance or someone needs to take over.

(m/validate
  [:or [:and [:map [:x [:= true]]]
             [:map [:y :int]]]
   [:map [:y :string]]]
  {:x true :y "oops"})
=> true

ah

I suck at logic

that should probably be

[:or [:map [:y :int]]
     [:and [:not [:map [:x [:= true]]]]
           [:map [:y :string]]]]

or if you want to make it even more explicit/watertight, you can check the condition on both sides of the :or EDIT: why the heck does slack autocomplete emoji inside code blocks???

> I suck at logic So do I. That's why I write tests 😄

this might be relevant as well: https://github.com/bsless/malli-keys-relations

or: https://github.com/metosin/malli/pull/1161

I think the forehead-acher here is that malli doesn't short circuit in same way as plain code would. The Clojure code matching the described logic works as expected, but as malli model the equivalent seems to be

[:or [:and [:map [:x [:= true]]]
           [:map [:y int?]]]
     [:and [:not [:map [:x [:= true]]]]  ; had to add negation here
           [:map [:y string?]]]]

yeah that looks good to me

I had a moment to leave a review.

me too

The trick/hard part seems to be that effectively this means adding

{
  "if": ...,
  "then": ...,
  "else": ...
}

it seems to me you could add a new schema :if to malli, used like this:

[:if [:map [:x [:= true]]]
     [:map [:y :int]]
     [:map [:y :string]]]

[:or [:and [:map [:x [:= true]]]
           [:map [:y :int]]]
     [:map [:y :string]]]

once you've experimented with something like that, we could consider adding it to malli experimental or malli core

... and if you need :allOf, that's provided by malli :hand:

Sounds reasonable 👍 Yeah I might spend a bit of time on this to see if I can slot this in, would be quite handy in general, I think.

yeah, having malli-level support for all the json-schema features is something we want

even if it means a bit of redundancy in the malli core schemas

Is there general convention for schema specific configuration keys? Depending on which spec/impl doc one reads the type: "object" key may be omitted or must be included, and I have this now behind conf key, but it's named really poorly 🙂 I think I can extract a shareable version of this tomorrow and then figure out malli contribution. Turns out it was not-as-hard-to-implement as I thought. Just need to add some prettification things such as better explain descriptions and whatnot.

no I don't think we have schema-specific conf keys, just per-schema-instance props

We can shave that yak in the upcoming PR 😄

For my part, I'm in crunch mode at work and then I'll be on vacation until August. I can get back to this with you then!

Yeah no hurry, the version we have internally works well enough for us so there's no immediate need for this. Besides, quite often giving just time alone may result in better overall results.

hammock

Created an issue for this https://github.com/metosin/malli/issues/1287 Spent some weekend time to actually move and pretty much reimplement the schema as part of malli instead of what is embedded in our app 😛 I'll open a PR for scrutinizing soon.

and the matching PR: https://github.com/metosin/malli/pull/1288 I'll be off for the rest of the day, it's Mothers' Day after all 🙂