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

Use built-in instrument checking machinery instead of custom assertions #9

Merged
merged 8 commits into from
Jul 13, 2020

Conversation

kennyjwilli
Copy link
Member

This library currently throws its own assertions when a functions args or ret does not pass the written spec. This PR switches to use the pre-built clojure.spec.test.alpha/instrument functionality. If Orchestra is on the classpath, the :ret will also be checked.

@kennyjwilli kennyjwilli self-assigned this Feb 19, 2020
@kennyjwilli
Copy link
Member Author

@Frozenlock Curious on your thoughts on this approach instead. This would be a breaking change but certainly seems a lot simpler and more general moving forward.

@Frozenlock
Copy link
Contributor

It looks simpler, but also introduces other dependencies.
In addition, like you said, it's a breaking change.

If I have to use Orchestra to check :ret, what's the point of this library? (ie why wouldn't I just use Orchestra?)

@kennyjwilli
Copy link
Member Author

No extra deps are necessary unless you want to check :ret. Orchestra is a very small library that has no deps itself. I don't think there's much friction to asking people to add it for :ret checks.

If I have to use Orchestra to check :ret, what's the point of this library? (ie why wouldn't I just use Orchestra?)

See note here

@Frozenlock
Copy link
Contributor

No extra deps are necessary unless you want to check :ret.

Sure, but I do check :ret. 😕

I don't understand why all those breaking changes wouldn't be in a separate library. But at the same time I'm not the maintainer and if they can make things easier for you, you might as well do it.

@kennyjwilli kennyjwilli merged commit 5454b59 into master Jul 13, 2020
@kennyjwilli kennyjwilli deleted the instrument-checking-fn branch July 13, 2020 16:45
@kennyjwilli
Copy link
Member Author

@Frozenlock fyi, this is the direction I'd like to take. It was merged and deployed in 0.2.0.

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.

None yet

2 participants