-
Notifications
You must be signed in to change notification settings - Fork 468
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
Can we reduce the amount of custom DynamicProxy exception types? #518
Comments
I definitely like the idea of using FCL exceptions (e.g. I recall DP also throws If you put together a PR probably using a single |
The pull requests were all great, I'm happy with all the changes. Thrown exceptions definitely follow a clean pattern now. |
@jonorossi, I'm glad you were happy with the changes. I couldn't quite shake the feeling that those PRs were a bit messy, but that was perhaps unavoidable, given the many case-by-case decisions necessary. It feels good to have those exceptions out of the way now! |
I didn't feel that way. The commit messages made it easy to follow your thought process. |
DynamicProxy currently defines four exception types:
GeneratorException
InvalidMixinConfigurationException
InvalidProxyConstructorArgumentsException
ProxyGenerationException
The uses of these exceptions can be roughly summarized as follows:
ArgumentException
orInvalidOperationException
could be thrown instead.In none of these circumstances would user code profit from catching the exceptions thrown by DynamicProxy, so there's really no point in having distinct exception types.
I may have glossed over some details in my summary above, nevertheless I would suggest that we reduce the amount of custom exception types in DP by (1) using standard FCL exception types where appropriate, and (2) using a single
ProxyGenerationException
(or perhaps better, a new, more generally namedDynamicProxyException
) for truly exceptional situations.(If we do that, we may get rid of
GeneratorException
and thus theCastle.DynamicProxy.Generators
sub-namespace in the public API. See #517 for the full argument.)The text was updated successfully, but these errors were encountered: