greglook / puget

Canonical Colorizing Clojure Printer
The Unlicense
252 stars 27 forks source link

Cljs support (WIP) #30

Closed venantius closed 5 years ago

venantius commented 8 years ago

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.

venantius commented 8 years ago

I have some thoughts:

  1. JavaScript does not have an inheritance model in the same way that Java does. We can check that a given object satisfies an interface, for instance, but there isn't really chained inheritance the way there is in Java.
  2. Checking for basic types is obviously going to be different. We would dispatch on js/Number rather than java.lang.Integer etc.
  3. I find the puget.dispatch namespace confusing.

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 class doesn't exist in ClojureScript, we'd end up adopting a very different approach - one based first on checking type, and then after that using satisfies? to check against known interfaces (possibly). Unlike in Java, we don't have the option of calling, .getInterfaces on the class itself.

Thoughts @greglook ?

greglook commented 8 years ago

JavaScript does not have an inheritance model in the same way that Java does.

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.

I find the puget.dispatch namespace confusing.

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.

greglook commented 8 years ago

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.

venantius commented 8 years ago

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?

venantius commented 8 years ago

Upstream PR for arrangement will need to happen for this as well: https://github.com/greglook/clj-arrangement/pull/1