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

Create Custom Destructurers for More Exceptions #418

Open
24 tasks
RehanSaeed opened this issue Oct 19, 2021 · 7 comments
Open
24 tasks

Create Custom Destructurers for More Exceptions #418

RehanSaeed opened this issue Oct 19, 2021 · 7 comments
Labels
enhancement Issues describing an enhancement or pull requests adding an enhancement. good first issue A good first issue for a new community member to take on. help wanted Help wanted from the community.

Comments

@RehanSaeed
Copy link
Owner

Describe the feature

To avoid using reflection we can create custom destructurers for more exception types. ArgumentExceptionDestructurer is a good example for how to do this.

  • System.AggregateException
  • System.BadImageFormatException
  • System.Diagnostics.Contracts.ContractException
  • System.Globalization.CultureNotFoundException
  • System.IO.FileLoadException
  • System.IO.FileNotFoundException
  • System.MissingFieldException
  • System.MissingMemberException
  • System.MissingMethodException
  • System.NotFiniteNumberException
  • System.ObjectDisposedException
  • System.Reflection.ReflectionTypeLoadException
  • System.Resources.MissingSatelliteAssemblyException
  • System.Runtime.CompilerServices.RuntimeWrappedException
  • System.Runtime.CompilerServices.SwitchExpressionException
  • System.Runtime.InteropServices.ExternalException
  • System.Security.SecurityException
  • System.Text.DecoderFallbackException
  • System.Text.EncoderFallbackException
  • System.Threading.AbandonedMutexException
  • System.Threading.Tasks.TaskCanceledException
  • System.Threading.ThreadAbortException
  • System.TypeInitializationException
  • System.TypeLoadException
@RehanSaeed RehanSaeed added enhancement Issues describing an enhancement or pull requests adding an enhancement. help wanted Help wanted from the community. labels Oct 19, 2021
@krajek
Copy link
Collaborator

krajek commented Oct 19, 2021

Just a question: is it worth it? The cost of reflection is negligible and paid just for the first time exception is destructured. The cost is also minuscule compared to the cost of logging anyway.
Supporting so many types of exceptions manually seems like a burden in terms of lines of code/tests/maintenance.

As was thinking even, maybe let's go the other way around, and remove all(or most) custom destructurers we have and leave just reflection-based one?

@RehanSaeed
Copy link
Owner Author

We only have a few custom destructurors at the moment. The list above just completes everything built into System.*. Some of the above are more important than others. I've added them all above for completeness.

The cost can add up if you have an app e.g. if you have an app throwing large numbers of TaskCanceledException exceptions for example which can happen in real life quite easily. There is an emphasis on performance in .NET lately, so I think it makes sense.

@RehanSaeed RehanSaeed added the good first issue A good first issue for a new community member to take on. label Nov 9, 2021
@Driedas
Copy link
Contributor

Driedas commented Feb 2, 2022

Hi @RehanSaeed, would you accept a PR that adds a destructurer for RegexMatchTimeoutException?

@RehanSaeed
Copy link
Owner Author

@Driedas Sure!

@Driedas
Copy link
Contributor

Driedas commented Feb 3, 2022

@RehanSaeed
heh, never mind, I've cloned the project, added the following test to prove that this isn't implemented yet:

var exception = new RegexMatchTimeoutException("input", "pattern", TimeSpan.FromSeconds(1));
Test_LoggedExceptionContainsProperty(exception, nameof(exception.Input), exception.Input);

Imagine my surprise when it turned green instead of red as expected :-). Looks like ReflectionBasedDestructurer already takes care of this using a bit of smart reflection. D'oh, TDD scores...

@RehanSaeed
Copy link
Owner Author

Yes, as you've found it works but requires reflection to output. I'd still accept a PR to add support without using reflection.

@RehanSaeed
Copy link
Owner Author

I believe we may be able to do this with a source generator. Worth looking into.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues describing an enhancement or pull requests adding an enhancement. good first issue A good first issue for a new community member to take on. help wanted Help wanted from the community.
Projects
None yet
Development

No branches or pull requests

3 participants