Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add integrant.spec namespace #38

Closed
wants to merge 1 commit into from
Closed

Add integrant.spec namespace #38

wants to merge 1 commit into from

Conversation

helins
Copy link

@helins helins commented Jan 23, 2018

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.

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.
@weavejester
Copy link
Owner

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.

@helins
Copy link
Author

helins commented Jan 23, 2018

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.

@weavejester
Copy link
Owner

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.

@helins
Copy link
Author

helins commented Jan 24, 2018

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.

@weavejester
Copy link
Owner

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)

@helins
Copy link
Author

helins commented Nov 12, 2018

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 ?

@weavejester
Copy link
Owner

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.

@helins
Copy link
Author

helins commented Nov 13, 2018

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.

@weavejester weavejester closed this Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants