-
Notifications
You must be signed in to change notification settings - Fork 212
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 support for uri? in malli.transform (Clojure only) #966
Conversation
(if (string? x) | ||
(try | ||
(URI. x) | ||
(catch Exception _ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to catch java.net.URISyntaxException
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your call. The body of the try
is so small there's little possibility of masking unrelated exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right that the body is small, but I prefer URISyntaxException
for explicitness - so I have updated it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that fails the Babashka job, so I guess I change it back :-)
https://github.com/metosin/malli/actions/runs/6493910884/job/17635777011
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked in #babashka on clojurians slack (thread here), and @borkdude said that he added it to babashka so it will be available in the next release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't get the recommended macro approach to work, so I just put a comment in to tighten up the catch clause once our version of babashka has the required commit.
612ac73
to
51e2c8c
Compare
With Reitit (0.7.0-alpha7), using the below route definition, I get a failure during the coercion when passing the uri "http://example.com". ["/uri" {:post {:summary "Post a URI - fails coercion" :coercion reitit.coercion.malli/coercion :parameters {:body {:uri uri?}} :responses {200 {:body {:uri uri?}}} :handler (fn [{{{uri :uri} :body} :parameters}] {:status 200 :body {:uri uri}})}}]] When I replaced the :coercion with the Clojure Spec version, the coercion works. With this change the Malli coercer should also work. I made it Clojure specific since it relies on `java.net.URI`.
good catch re: babashka! |
With Reitit (0.7.0-alpha7), using the below route definition, I get a failure during the coercion when passing the uri "http://example.com".
When I replaced the :coercion with the Clojure Spec version, the coercion works.
With this change the Malli coercer should also work.
I made it Clojure specific since it relies on
java.net.URI
.