-
Notifications
You must be signed in to change notification settings - Fork 27
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
Cljs support (WIP) #30
Conversation
I have some thoughts:
I don't really understand how the library works right now, so just at a high level I'd like some clarification that I really understand what's going on. My understanding of the code is as follows:
It would seem to me that since Thoughts @greglook ? |
This is going to be a little different, but I think the cljs code should check direct implementations/protocols rather than worrying about inheritance. That should solve most of the use-cases.
A lot of the code is for logical dispatch, to factor out common patterns for doing inheritance-style lookups in Java land. In a way, it's a simplified version of Clojure's multimethod/hierarchy dispatch. |
@@ -19,8 +19,9 @@ | |||
{:pre [(sequential? dispatchers)]} | |||
(let [candidates (remove nil? dispatchers)] | |||
(when (empty? candidates) | |||
(throw (IllegalArgumentException. | |||
"chained-lookup must be provided at least one dispatch function to try."))) | |||
#?(:clj (throw (IllegalArgumentException. |
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.
For exceptions, I think a better route is to use the platform-agnostic ex-info
constructor.
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.
TIL
It seems like a lot of the changes are format-related - please don't change the existing code style. It also makes the diff harder to read. |
Yeah, sorry, I just ran cljfmt on the project and it changed a bunch of things. Would you be open to a separate alignment PR for the cljfmt stuff? |
Upstream PR for arrangement will need to happen for this as well: greglook/clj-arrangement#1 |
This is just a start; there's a bunch of stuff that'll need to be changed and I wanted to go through some of it in a somewhat interactive form rather than going too far down the rabbit hole and finding out that wasn't a direction we agreed on.