Skip to content
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

Extract more detail from compiler exceptions #262

Merged
merged 1 commit into from
Oct 9, 2015

Conversation

mainej
Copy link

@mainej mainej commented Sep 25, 2015

This patch re-introduces the inner 'cause' of compiler exceptions.

These exceptions are not always informative, which may be why they were
hidden in 0a57231. For example when the compiler is complaining about
a syntax error, the exception often refers to a line number far from the
actual syntax problem.

However, the exceptions can be quite useful, for example when they
complain about an undeclared variable.

This patch re-introduces this additional detail. I'd be equally happy
with completely removing extract-location. The output from the two
approaches is similar, and perhaps less code is better.

@@ -104,7 +104,7 @@
[{:keys [class] :as cause}]
(if (= class "clojure.lang.Compiler$CompilerException")
(update-in cause [:message] str/replace
#".* (compiling:)\((.*)\)" "Error $1 $2")
#".*?: (.*?), (compiling:)\((.*)\)" "Error $2 $3 $1")
Copy link
Member

Choose a reason for hiding this comment

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

Isn't .*? the same as .*

Copy link
Author

Choose a reason for hiding this comment

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

Nope, slightly different [1]. In this case, you would probably be safe with either variation, but I'm being paranoid.

user=> (clojure.string/replace-first "a:b c:d" #"(.*):(.*)" "$1x$2")
"a:b cxd"
user=> (clojure.string/replace-first "a:b c:d" #"(.*?):(.*)" "$1x$2")
"axb c:d"

[1] http://www.regular-expressions.info/repeat.html#lazy

@mainej
Copy link
Author

mainej commented Sep 25, 2015

Looks like all the Travis builds have been failing recently... This commit fails the same part of the build matrix that other recent commits fail.

@expez
Copy link
Member

expez commented Sep 26, 2015

This looks good to me. It would be nice to have a test for this, though.

Looks like all the Travis builds have been failing recently...

Yes, this is quite embarrassing :/

@mainej
Copy link
Author

mainej commented Sep 28, 2015

Passing tests added, though Travis is still failing in part of the build matrix.

@jeffh
Copy link

jeffh commented Oct 7, 2015

Any updates on this PR? I'd love this to be merged in :)

@bbatsov
Copy link
Member

bbatsov commented Oct 7, 2015

The commits have to be squashed together. Apart from that it looks ok.

@mainej
Copy link
Author

mainej commented Oct 8, 2015

Commits squashed, though Travis is still failing in part of the build matrix.

@bbatsov
Copy link
Member

bbatsov commented Oct 8, 2015

Did you rebase your code on top of the current master. The tests are no longer failing there.

This patch re-introduces the inner 'cause' of compiler exceptions.

These exceptions are not always informative, which may be why they were
hidden in 0a57231. For example when the compiler is complaining about
a syntax error, the exception often refers to a line number far from the
actual syntax problem.

However, the exceptions can be quite useful, for example when they
complain about an undeclared variable.

This patch re-introduces this additional detail. I'd be equally happy
with completely removing `extract-location`.  The output from the two
approaches is similar, and perhaps less code is better.
@mainej
Copy link
Author

mainej commented Oct 8, 2015

Ah, rebased... tests now passing.

expez added a commit that referenced this pull request Oct 9, 2015
Extract more detail from compiler exceptions
@expez expez merged commit 54d4c57 into clojure-emacs:master Oct 9, 2015
@expez
Copy link
Member

expez commented Oct 9, 2015

Thanks @mainej 👍

@bbatsov
Copy link
Member

bbatsov commented Oct 9, 2015

@jeffvalk btw, why did you opt to filter those out?

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

Successfully merging this pull request may close these issues.

None yet

5 participants