search

noprompt / ankha

A data inspection component for Om
Eclipse Public License 1.0
132 stars 12 forks source link

Fix problem parsing non-sequable date-time (goog.date.UtcDateTime). #25

Open bilus opened 9 years ago

bilus commented 9 years ago

When inspected cursor contains a UtcDateTime object, call to empty? throws an exception. This is a quick workaround for this.

noprompt commented 9 years ago

I could be mistaken but this seems like the problem is further upstream, no? We only ask if something is empty? in one of the collection view components. Perhaps goog.date.UtcDateTime should have and implementation of IInspect since it's not really a collection?

bilus commented 9 years ago

This maybe so. In any case, this quick & dirty fix-kind-of-thing worked for me and I thank you for the inspector, it's really really useful.

As a side node, ankha skips stuff it doesn't understand or doesn't want to handle (e.g. functions) so I think this workaround does follow its spirit. I think a tool like this should just do its best to "just work" out of the box. YMMV

noprompt commented 9 years ago

As a side node, ankha skips stuff it doesn't understand or doesn't want to handle (e.g. functions) so I think this workaround does follow its spirit.

The default behavior is pr-str. Thinking of that as "skipping" is probably not the best frame of mind. If the code is giving that impression then I feel like I've failed on some level as a programmer. Really, I thought pr-str was just a good default.

The problem with the goog.date.UtcDateTime is that it's being handled by one of the collection viewing components and it shouldn't be. This is why I made the recommendation to extend the protocol to the type instead of patching empty?. The patch for empty?, as it stands, is overly permissive and it is addressing the problem in the wrong place.

I think a tool like this should just do its best to "just work" out of the box.

I'm in complete agreement. But let's solve the right problem.

bilus commented 9 years ago

I stand corrected about pr-str and the approach. Yes, my patch sucks; you obviously know the code waay better than I do. What I was trying to do is just get the inspector work in our project and make it stop crashing as quickly as possible.

The problem is certainly wider than just UtcDateTime as you seem to have taken into consideration when writing inspect so while extending IInspect is certainly the right thing to do, I think there should be a built-in way of handling at least Closure Library types.

Currently (without extending the type to handle the IInspect protocol) (satisfies? IInspect goog.date.UtcDateTime) returns true (I didn't have the time to dig deeper) which results in coll-view being rendered which is clearly incorrect and what you get is an uninformative exception in the js Console. A bit confusing for someone who doesn't have the time to read the sources of ankha to figure out what is going on.

noprompt commented 9 years ago

The satisfies? check returning true seems busted. I'm guessing the problem lies with the object implementation. Maybe what we could do is move this code to here and handle the object? case specially. I think what we want to do is distinguish the difference between a vanilla #js{} and something like goog.date.UtcDateTime. Vanilla objects should be rendered by coll-view and everything else should implement IInspect. That should give the fast failure you're looking for.

bilus commented 9 years ago

This could be a solution. Still, there are more issues relating to that e.g. editing doesn't work (i.e. you cannot save because the edn is not parsable).

On Wed, May 20, 2015 at 8:01 PM, Joel Holdbrooks notifications@github.com wrote:

The satisfies? check returning true seems busted. I'm guessing the problem lies with the object implementation. Maybe what we could do is move this code https://github.com/noprompt/ankha/blob/master/src/ankha/core.cljs#L492-L493 to here https://github.com/noprompt/ankha/blob/master/src/ankha/core.cljs#L103 and handle the object? case specially. I think what we want to do is distinguish the difference between a vanilla #js{} and something like goog.date.UtcDateTime. Vanilla objects should be rendered by coll-view and everything else should implement IInspect. That should give the fast failure you're looking for.

— Reply to this email directly or view it on GitHub https://github.com/noprompt/ankha/pull/25#issuecomment-103975490.