Fork me on GitHub
#clojure-dev
<
2017-07-04
>
mpenet13:07:32

Would love to get some feedback on https://dev.clojure.org/jira/browse/CLJ-2194 - I might dedicate some time to work on it if I get directions

rickmoynihan00:07:17

mpenet: FWIW this looks good to me. One thing that did occur to me though is that the option you present about registering metadata in a separate registry (`atom`) might lead to race conditions as the spec atom changes before/after the metadata atom. For this reason it seems it might be better to either: 1. reuse the existing spec registry (as you suggest) by adopting a metadata key in it. 2. make the spec and metadata registries refs and apply a dosync transaction across them. 3. Adopt the meta protocol method you suggest.

mpenet07:07:43

If I could choose I would adopt the meta protocol. Seems that the changes required are not so bad and it seems like the cleanest solution.

mpenet07:07:49

This ticket would make all the libs currently implementing custom Specs (to add some metadata to their specs) life way easier. Ex speculate, and all the swagger related libs, it would basically make specs more extensible without having to bother people on jira. It should get more attention imho.

rickmoynihan08:07:59

agree entirely on the utility of metadata. I ran into the doc string issue and saw there was a JIRA for doc strings but not metadata, so raised it on slack and was pleased to see you argue for metadata. It seems pointless to me to provide just docstring support, when metadata will have so many other uses. And I’d also be inclined towards using IMeta as otherwise we’ll end up with two different ways to read metadata from things, one for specs the other for everything else. One small issue is that keywords could not implement IObj and with-meta because (identical? :foo :foo) ;; => true. I realise you’ve not mentioned this in the ticket, and weren’t proposing to implement IObj. However it’s worth mentioning that this might mean keywords would be a special case and wouldn’t be symmetrical like (most, probably all?) other clojure objects.

rickmoynihan08:07:46

symmetry isn’t really a hugely important design goal though… and I note you’ve clearly thought a little about this too because (s/def ::foo map? {:doc "foo docs"}) doesn’t mirror def e.g. (s/def ^{:doc "foo docs"} ::foo map?) which would clearly be the first choice if the above wasn’t an issue.

mpenet09:07:15

I mentioned this behind the need to reify spec "pointers" ex (s/def ::foo ::bar), right now in the registry they are just a kw value, but they could be reified impl of Spec that points to the other spec but have their own metadata (the rest of the functions would just call the other specs, no copying of anything)

mpenet09:07:24

maybe it's not clear

mpenet09:07:20

I am certainly missing important details, but that seems like a decent solution

rickmoynihan14:07:19

Yeah I meant to ask what you meant by pointers, that bit didn’t read very clearly to me… I’m not entirely sure I understand the solution you’re proposing there though. Are you 1) proposing to store the metadata in the keyword value, or are you proposing to 2) store it in the spec registry and have (meta ::foo) look itself (`::foo`) up in the global database? I’m guessing it must be 2 - but I’d originally read it as 1.

mpenet14:07:31

keywords do not support metadata, so basically right now when you have a spec such as (s/def ::foo ::bar) it will store something like {::foo ::bar, ...} in the registry, I am saying we could store {::foo S, ...} where S would be a reified instance of the Spec protocol, with all the function such as explain conform etc deferring to the original spec ::bar points to, and it's own metadata.

mpenet14:07:54

that's what I mean by wrapping the keyword into a reified instance of Spec, that would just be a "container" with references to the original (basically it would call the Spec fn of the "parent") + potentially instance specific stuff such as metadata

rickmoynihan14:07:24

ok that makes a lot more sense…. so basically (s/def ::registry (s/map-of namespaced-keyword? ::spec-obj)

rickmoynihan14:07:30

(s/def ::spec-obj (s/and #(satisfies? IMeta %) ,,,)

mpenet14:07:45

that's the basic idea, then it can be optimized in a few ways

cgrand14:07:43

Local variable declarations are missing for invoke methods that just delegate to invokeStatic, causing confusion in debuggers. Ok to open an issue?

andy.fingerhut15:07:03

@mpenet I don't have any feedback other than there is also this issue, which Alex Miller has said they aren't looking for a patch because they don't have a recommended approach yet: https://dev.clojure.org/jira/browse/CLJ-1965