Open jasonjckn opened 1 year ago
Thanks for reporting. satisfies?
is dead-slow and support for it was removed in the schema creation performance improvement epic.
Please go vote up https://clojure.atlassian.net/browse/CLJ-1814.
one option would be to first try the instance?
call (should match ~100% of cases) and use satisfies?
as backup. Commented on the other issue already (https://github.com/metosin/malli/issues/961#issuecomment-1732537677) - malli supports Class Schemas out via registry, bit more code but explicit control.
do you still see that backup call to satisfies?
would be needed here?
one option would be to first try the instance? call (should match ~100% of cases) and use satisfies? as backup.
Thanks for the context re: CLJ-1814
My specific use case here... which is
(extend-protocol m/IntoSchema
java.lang.Class
(-type [^Class this] (symbol (.getName this)))
I might backtrack on my use case, i'll leave a comment @ https://github.com/metosin/malli/issues/961
I'm kind of split 50/50 on this now, I still think extend-protocol m/IntoSchema
is a valid use case, one expects extend-protocol to work, if not for java.lang.Class, then I'm sure I'll find utility for this down the road if not today.
I'm very sympathetic to performance, maybe if i change this code this way?
If you feel good about that... or something else, i can send a PR, if not maybe its best if I just close this and reopen later.
I think I realized the right solution here, it's not the most elegant code but it accomplishes all the performance objectives and retains open polymorphism / extensible to any type such that into-schema?
is open instead of closed — the way it is today. Just make into-schema?
a protocol instead of calling satisfies?
or even instance?
Thus any time you extend the protocol m/IntoSchema, you would also have to extend m/SupportsIntoSchema,
I think if you go this route, it may simplify into-schema
function as well, because you'll be able to do (extend-protocol m/IntoSchema IPersistentVector
instead of handling it as a special case .... but if you don't care for this specific part, no worries.
shall I send a PR?
P.S. I wonder why clojure core team didn't implement satisfies?
like this from the get-go.
I guess i could have also supported java.lang.Class by defining a protocol 'RegistryKeyLookup', and extend it for java.lang.Class , and java.lang.Object fallback to do an actual Registry lookup.
but nonetheless i feel like the above is still a nice design... especially if you extend protocol for IPersistentVector, it'll perform about the same, but a bit more modular,
would become basically just
could even perform a tiny bit faster
This function definition should call
clojure.core/satisfies?
notinstance?
I was trying to write code like
And I wasn't getting the desired behaviour, because
If someone can confirm this is unintentional, i'll submit a PR.