search

sethtrain / raven-clj

A Clojure interface to Sentry
84 stars 29 forks source link

Use parent class java.lang.Throwable #26

Closed tangrammer closed 5 years ago

tangrammer commented 5 years ago

Remove duplicated catching approach

martinklepsch commented 5 years ago

@tangrammer Hey! Thanks for opening a PR and helping to maintain this library :)

A cursory look at Java docs seem to indicate that Exception is not a subclass of Error. I've previously caught Throwable when I wanted to catch exceptions and asserts but I think that's also not really recommended.

Can you explain a bit more what motivated this change?

tangrammer commented 5 years ago

sorry i wanted to use Throwable 😄 ... I've edited the PR but i had no idea about why is not recommended ... 🤔

tangrammer commented 5 years ago

reading a bit about the reasoning behind not using Throwable it seems to me that it doesn't apply to current code thus it throws the exception at the end 🤔

martinklepsch commented 5 years ago

Can you explain why this change is necessary? I see that it catches a broader set of Error instances but I'm hesitant to just add it "for the sake of it". Feedback by other users of this library is welcome as well of course.

tangrammer commented 5 years ago

IMO the change is not necessary as far as it doesn't change the behavior at all, it could be considered a code improvement if you consider it... anyway don't think it deserves so much discussion ... feel free to close the PR at any time :)