Conversation
|
do you want an initial review of this already or is it still under progress? |
|
@opqdonut an initial review would be good. The summary is: this attempts to reuse objects in memory as much as possible without changing the arguments to Open question: is this schema reuse a way to remove |
opqdonut
left a comment
opqdonut
left a comment
There was a problem hiding this comment.
I feel like the complexity is growing a lot here, but I'm not completely opposed to merging something like this. I think we might benefit from a design doc that talks about how the caching works, with comments in the impl pointing to that doc. That way once this theme has been forgotten, we don't accidentally break things while touching these lines.
| (-type [_] :ref) | ||
| (-type-properties [_] type-properties) | ||
| (-into-schema [parent properties [ref :as children] {::keys [allow-invalid-refs] :as options}] | ||
| (-into-schema [parent properties [ref :as children] {::keys [allow-invalid-refs ref-id->nested-info] :as options}] |
There was a problem hiding this comment.
What will the contents of nested-info be? Could be documented somewhere. I see at least :rf going in there but do you envision something like :validator being there as well?
| rf (-memoize #(schema (?schema) options)) | ||
| (let [id (-identify-ref-schema ref options) | ||
| nested-info (get ref-id->nested-info id) | ||
| rf (or (:rf nested-info) |
There was a problem hiding this comment.
I wonder if a better name than rf could now be chosen. Maybe something like deref-this? construct-child-schema?
| (-validator [this] | ||
| (let [id (-identify-ref-schema this) | ||
| id->validator *ref-validators*] | ||
| (let [id->validator *ref-validators*] |
There was a problem hiding this comment.
I feel like the complexity with both ref-id->nested-info and *ref-validators* is significant. Could they be combined somehow?
| (is (= ["hello" "maailma"] | ||
| (mapv (comp peek m/form m/deref-all) (m/children schema))))))) | ||
|
|
||
| (deftest recursive-explainer-test |
There was a problem hiding this comment.
thanks for the very concrete test
2 participants
Avoids recreating child schemas for recursive explainers.