integrant icon indicating copy to clipboard operation
integrant copied to clipboard

Add integrant.spec namespace

Open helins opened this issue 7 years ago • 8 comments

Sorry for the delay ! This pull request is related to issue #32 for adding the 'integrant.spec' namespace. My solution is slightly different, the ::ig.spec/configuration spec replaces your idea of ig.spec/derived-keys as there is no need for a function, but the goal is the same. In order to work well enough with s/explain, I used s/multi-spec in a bit of an unconventional way. If you like it, I can write tests. For the time being, let's say it's "repl proven".

This namespace aims to provide utilities for clojure.spec, specially regarding validation of configuration maps.

Notably :

  • integrant.spec/ref creates specs for refs.
  • :integrant.spec/configuration validates a configuration map by finding the spec for every key in the global registry. It takes into account derived and composite keys. Also, it checks for ambiguous refs.

helins avatar Jan 23 '18 17:01 helins

I think I'd still prefer derived-keys over a keyword specification. Using multi-spec might be a good idea; I'll take a look at how it operates.

The functions that do one thing can be replaced with anonymous functions, or with higher level functions (e.g. (constantly true)). Private functions shouldn't require docstrings.

You also might have a problem with your editor, because all the new lines are doubled up.

weavejester avatar Jan 23 '18 17:01 weavejester

I actually use a lot of space which looks very clear in my editor but totally weird on github. I like to formalize private fns that do only one thing. But those are a matter of style and will be modified to match more closely 'integrant.core' if you approve the code, so at least there would be coherence.

For the time being, I suggest you to focus on how ::ig.spec/configuration behaves and if you like it, we can discuss how to present it. As far as I'm concerned, it does the job. I will incorporate this to my current projects this week to get a better feedback.

helins avatar Jan 23 '18 18:01 helins

I'd rather a derived-keys function is used over a ::configuration keyword, as then we can supply required keys via a :req option. As un-namespaced keywords cannot be derived, there is no need for a :req-un option.

The ref function should use ref? rather than reflike?, as the two are not interchangable. A ref returns a single value; a refset returns a set. There should be a refset function to mirror ref.

weavejester avatar Jan 23 '18 19:01 weavejester

Agreed on ref, but what is refset really used for ?

Regarding derived-keys, I agree we should be able to enforce required keys. But the rest would be as it is, in my opinion. You don't have to supply anything else for ::configuration to do its job as it tries to find specs in the global registry while respecting the idea of derived and composites keys.

helins avatar Jan 24 '18 08:01 helins

refset is used to generate a set of refs. It's useful for times when you want to reference all keys that derive from a base key, either because you want to do something to their values, or to guarantee a dependency order.

Regarding ::configuration, I'm leaning toward a config function instead. My reasoning is that, unlike say a Ring request map, an Integrant configuration is more user specific. Adding in required keys as an option is the most obvious customization, but there might be other things we want to test in future as well.

So perhaps something like:

(s/unambiguous-refs? config)
(s/no-missing-refs? config)
(s/derived-keys & {:keys [req opt]})
(s/ref key)
(s/refset key)
(s/config & opts)

weavejester avatar Jan 24 '18 15:01 weavejester

Sorry for the (very) long delay, I was super busy with work. I did not follow what happened with integrant meanwhile but it looks like this PR might still be of interest. Did you have requests for that kind of things ?

helins avatar Nov 12 '18 12:11 helins

I haven't received any more requests for compatibility with spec. I think it would be nice to have, but somewhat complex to implement, so it hasn't been a priority of mine.

weavejester avatar Nov 12 '18 13:11 weavejester

Well if nobody is clearly interested in that, I will leave it as it is for the time being. Yes, I admit it is less straightforward to implement than I first thought.

helins avatar Nov 13 '18 10:11 helins