Skip to content
This repository has been archived by the owner on Oct 21, 2022. It is now read-only.

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

Merged
merged 1 commit into from
Mar 16, 2016

Conversation

carocad
Copy link
Contributor

@carocad carocad commented Feb 23, 2016

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.

```Clojure
; for Clojure
[:editor.clj :-lt.plugins.clojure/clj-exception]
[:editor.clj :lt.plugins.user/clj-expandable-exception]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong namespace referred to

@rundis
Copy link
Contributor

rundis commented Feb 25, 2016

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
Copy link
Contributor Author

carocad commented Feb 29, 2016

@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.

```Clojure
; for Clojure
[:editor.clj :-lt.plugins.clojure/clj-exception]
[:editor.clj :lt.plugins.clojure/clj-expandable-exception]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you are missing collapsible-exception in the namespace here

@rundis
Copy link
Contributor

rundis commented Mar 3, 2016

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
Copy link
Contributor Author

carocad commented Mar 16, 2016

@rundis any thoughts about the current proposal?

@rundis
Copy link
Contributor

rundis commented Mar 16, 2016

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
Copy link
Contributor Author

carocad commented Mar 16, 2016

@rundis done

@rundis
Copy link
Contributor

rundis commented Mar 16, 2016

@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.

README updated. collapsible.exception TAG added
@carocad
Copy link
Contributor Author

carocad commented Mar 16, 2016

@rundis you meant that?

rundis added a commit that referenced this pull request Mar 16, 2016
collapsible exceptions added in a new namespace, according to #77
@rundis rundis merged commit 76c9338 into LightTable:master Mar 16, 2016
@rundis
Copy link
Contributor

rundis commented Mar 16, 2016

tx for your contribution @carocad !

@kenny-evitt
Copy link

@carocad @rundis Nice!

@carocad
Copy link
Contributor Author

carocad commented Apr 7, 2016

@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))

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants