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.

Sign up for GitHub

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

Jump to bottom

Add ifn? schema #416

Merged
merged 1 commit into from
Apr 13, 2021
Merged

Add ifn? schema #416

merged 1 commit into from
Apr 13, 2021

Conversation

jcf
Copy link
Contributor

@jcf jcf commented Apr 13, 2021

As per a thread over on the Clojurians' Slack group, I've teed up a first draft at adding support for ifn? in Malli schemas.

(m/validate ifn? :kw)
(m/validate ifn? (fn [] :ok!))

This is my first time contributing so apologies for any faux pas. I think I've covered all of the bases with transformers, generators, JSON schema, humanized errors, and clj-kondo, but would really appreciate some testing and pointers from the more familiar Malli contributors.

Thank you for sharing this awesome library, and all of the Metosin suite. It's truly awesome stuff!

@jcf
Copy link
Contributor Author

jcf commented Apr 13, 2021

Happy to rebase things and extend this with anything I've missed. There was mention of adding support for {:clj-kondo/type :ifn} to the Malli API, which I've not gotten around to. 🙈

@jcf
Copy link
Contributor Author

jcf commented Apr 13, 2021

Oops! I broke the Clojurescript build with some use of reify and IFn:

FAIL in (validation-test) (cljs/test.js:433:14)
ifn schemas
expected: (true? (m/validate schema (reify clojure.lang.IFn (invoke [_] "Invoked!"))))
  actual: (not (true? false))

I'll take a look at applying some reader conditionals after lunch.

ikitommi
ikitommi reviewed Apr 13, 2021
View reviewed changes
Copy link
Member

@ikitommi ikitommi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! a small ordering note if you have time for it. Also, please update the README and CHANGELOG of this addition.

Will merge this when the tests pass.

src/malli/clj_kondo.cljc
@@ -53,6 +53,7 @@
(defmethod accept 'sequential? [_ _ _ _] :sequential)
(defmethod accept 'ratio? [_ _ _ _] :int) ;;??
(defmethod accept 'bytes? [_ _ _ _] :char-sequence) ;;??
(defmethod accept 'ifn? [_ _ _ _] :ifn)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please align all the 'ifn? mappings after the fn? in all the relevant places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved any ifn? code after corresponding fn? code in both the implementation and tests. If there's any more clean up you'd like me to do, happy to do so.

@jcf
Copy link
Contributor Author

jcf commented Apr 13, 2021

Thanks for the feedback, @ikitommi! I’ll implement your requested changes and get the build passing very soon. 🙇‍♂️

@jcf
Copy link
Contributor Author

jcf commented Apr 13, 2021

ClojureScript tests pass for me locally (I ran bin/node), and I've added something to both the CHANGELOG and README.

If there's anything else I can do, please let me know!

jcf
jcf commented Apr 13, 2021
View reviewed changes
CHANGELOG.md
@@ -14,6 +14,10 @@ We use [Break Versioning][breakver]. The version numbers follow a `<major>.<mino

Malli is in [alpha](README.md#alpha).

## 0.5.0 (2021-04-13)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure what to put here so guessed this would be a minor version bump rather than just a patch.

The date comes across as quite presumptuous of me, and can absolutely be changed! 🙈

jcf
jcf commented Apr 13, 2021
View reviewed changes
README.md
(s/validate ifn? 123)
; => false
```

Copy link
Contributor Author

@jcf jcf Apr 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure of the best place to add this information. Hope it's okay.

@jcf jcf requested a review from ikitommi April 13, 2021 14:25
@ikitommi ikitommi merged commit 553e703 into metosin:master Apr 13, 2021
@ikitommi
Copy link
Member

Thanks!

@jcf jcf deleted the add-ifn-schema branch April 13, 2021 15:15
@jcf
Copy link
Contributor Author

jcf commented Apr 13, 2021

Thank you, @ikitommi! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ikitommi ikitommi Awaiting requested review from ikitommi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jcf @ikitommi