LightTable / Clojure

Light Table Clojure language plugin
MIT License
99 stars 47 forks source link

collapsible exceptions added in a new namespace, according to #77 #79

Closed carocad closed 8 years ago

carocad commented 8 years ago

Hey guys, Here is the implementation of collapsible exceptions for Clojure(script) according to #77. I'm not completely sure about the correct implementation of the Clojurescript exception behavior as I have not properly tested it, but in principle it should work the same as with Clojure.

rundis commented 8 years ago

The truncated results in clojurescript, doesn't seem to truncate well so I get a very very wide inline error. Might be some corner cases. Try evaling something like this i a cljs editor: (// 1 1)

carocad commented 8 years ago

@rundis I changed the files that you commented.

Regarding the very wide inline error. It not exactly a corner case, but a remainder of the previous inline-exception behavior. The original cljs behavior used the stacktrace as summary and inline-text simultaneously. The precise lines are here: https://github.com/carocad/Clojure/blob/error_proposal/src/lt/plugins/clojure/collapsible_exception.cljs#L81-L90

msg (or (:stack res) (:ex res))

From what I could check, sometimes the stacktrace is not "summarized" but taken entirely. In those cases there is no difference between the msg and stack but since there is no line break for the span tag, it all just ends up in a very wide line.

I think we could truncate the summary; taking only (:ex res) would leave the summary empty in some cases.

rundis commented 8 years ago

Tx for doing this and sorry for not being quicker to respond. Too many things going on. But we will get there. I think that apart for my minor comment on truncation, we are pretty much good to go :-)

carocad commented 8 years ago

@rundis any thoughts about the current proposal?

rundis commented 8 years ago

Looks good.

Could I just ask you for 2 more things and I'll merge;

  1. Remove the compiled js files (.js and .js.map). We'll compile before releasing a new version of the plugin
  2. Squash the commits
carocad commented 8 years ago

@rundis done

rundis commented 8 years ago

@carocad Almost. When I said remove the .js and .js.map file I meant don't include them in the commit. If I merge now the existing files will be deleted on master.

carocad commented 8 years ago

@rundis you meant that?

rundis commented 8 years ago

tx for your contribution @carocad !

kenny-evitt commented 8 years ago

@carocad @rundis Nice!

carocad commented 8 years ago

@rundis could you please check if the truncation works on your clojurescript? The case that we talked about (\\ 1 2) is not being truncated on my Clojurescript. But I'm not sure if it is only because of my user modifications. I think though that I truncated the wrong variable (see here)

I think it should be msg (or (truncate (:stack res)) (:ex res))