Conversation
SimonCropp
requested review from
bruno-garcia and
mattjohnsonpint
as code owners
| /// Recursively enumerates all <see cref="AggregateException.InnerExceptions"/> and <see cref="Exception.InnerException"/> | ||
| /// Not for public use. | ||
| /// </summary> | ||
| public static IEnumerable<Exception> EnumerateChainedExceptions(this Exception exception, SentryOptions options) |
There was a problem hiding this comment.
This not being for public use but part of the global namespace don't go well together.
Should we remove the note on the xml doc?
Or perhaps this is best not being an extension method of Exception? If it's under a different namespace it will already help not show up on the user's intellisense. The goal is to share the built-in behavior with the Unity IL2CPP support
There was a problem hiding this comment.
I'm ok with it as is, since it includes a SentryOptions parameter - we're not likely to step on anyone. We can hide it from intellisense with EditorBrowsable
| yield return inner; | ||
| } | ||
| } | ||
| return exception.EnumerateChainedExceptions(_options) |
There was a problem hiding this comment.
I was thinking we could refactor out this class to have a base class, so that the code that follows it doesn't need to iterate on the exceptions once again:
Then a third time to have the original + the Sentry exception side by side:
There was a problem hiding this comment.
@bruno-garcia i dont really understand what you are proposing here. can we sync my tuesday morning?
There was a problem hiding this comment.
I thought of re-thinking these hooks but didn't get that far, #1734
bruno-garcia
left a comment
bruno-garcia
left a comment
There was a problem hiding this comment.
Unless @mattjohnsonpint disagrees, we can go ahead with this
mattjohnsonpint
left a comment
mattjohnsonpint
left a comment
There was a problem hiding this comment.
Approved, with suggestions
Co-authored-by: Matt Johnson-Pint <matt.johnson-pint@sentry.io>
3 participants
No description provided.