ironcev / SwissKnife

[The description is yet to come]
7 stars 1 forks source link

Exception message unwrap #7

Open hudo opened 10 years ago

hudo commented 10 years ago

Can we implement extension method that unwraps exception, for example:

catch(Exception e) { message = e.Unwrap().Message; }

method traverse inner exceptions until it finds the last one, and returns it. It can be then used to get error message and display it, ...

For this, maybe we could add folder or class for exception related stuff? I can implement this and create pull request!

ironcev commented 10 years ago

Very nice idea, I like it :-) Also, thank you for your wish to implement it! Help is always welcome :-)

Before we implement anything I would like to sketch/design the class. I like the name "Unwrap()". It's cool :-) But honestly, I'm not sure if it will be intuitive enough. Things that we put to public API should be obvious :-)

How about something like this:

public class ExceptionExtensions
{
  IEnumerable<Exception> GetInnerExceptions(this Exception); // From last to first? Or other way around?
  Option<Exception> GetDeepestInnerException(this Exception);
}

Tell me what you think.

Thanks once again for the enhancement proposal.

ironcev commented 10 years ago

Ups, closed by mistake :-)

ironcev commented 10 years ago

Also, it would be good that the API is consistent with the API of the AggregateException class. That class have methods like Flatten() or properties like InnerExceptions().

We should also carefully consider what to return in our case if one of the inner exceptions is AggregateException.

Plenty of things to think about :-)

hudo commented 10 years ago

Ok, this started as simple solution for simple problem, now we have enterprise solution for all problems:) Ok, maybe not for all, just enterprise solution for simple problems! Just kidding. Even method name Unwrap sounds simple, nice and self-explanatory, if naming convention of SwissKnife is more like "GetDeepestInnerException" then we should go with that one. And for AggregateException use cases, right now I don't have clear picture how it would fit into this unwrapping of inner exceptions scenario... Those exceptions in AggregateE. is flattened list, and this Unwrap is more for deep nesting, which often happens. With that said, GetDeepest* name would imply traverse through InnerExceptions, not flattened list, and would maybe suits it best. For dealing with aggregate stuff, lets try to think of any real world scenario, and how can it be simplified with extension method(s).

And, one day, if we became famous because of this, i suggest that this pattern is named "matryoshka principle" :)

ironcev commented 10 years ago

:-) :-)

Jokes aside, it is very important for public API to have clearly defined semantics. This is particularly tricky in edge and not so obvious but still regular cases.

Out of this what we discussed so far I would go for the following:

public class ExceptionExtensions
{
  // AggregateExceptions are not specially handled. They are exceptions same as all others.

  // From the last inner exception to the first (deepest) on.  
  IEnumerable<Exception> GetInnerExceptions(this Exception);
  Option<Exception> GetDeepestInnerException(this Exception);
}

What bugs me still is the Exception.GetBaseException() method. According to its definition, it does exactly the same as our GetDeepestInnerException() doesn't it? So, why our extension method then?

ironcev commented 10 years ago

The question of Exception.GetBaseException() aside I've just realized that the proposal writen above by me is actually completely wrong :-) GetDeepestInnerException() is not what you want. What you need is the root cause of the exception. In case there is no inner exception, that is the exception itself. Your original Unwrap() method had that semantic I assume. Therefore the name GetDeepestInnerException() does not fit.

But again, what is the difference between the Unwrap() and Exception.GetBaseException()?

hudo commented 10 years ago

Unwrap() == GetBaseException(), or isn't? Unwrap is still much nicer name, and I saw people use it with that name. For now, I think we need only one extension method, Unwrap or with some other name (I like unwrap the most, but I'll leave you to decide).

ironcev commented 10 years ago

If I got you right, the Unwrap() has exactly the same semantics as the GetBaseException() but you still suggest to introduce it because it has "much nicer name"?

hudo commented 10 years ago

Yes. I don't like VB-like long method names, but as I said, if that's SK convention, we should then go with the long one.