-
Notifications
You must be signed in to change notification settings - Fork 455
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
Fixed first-chance HandlerException for optional parameters #450
Fixed first-chance HandlerException for optional parameters #450
Conversation
handler = TryGetHandlerFromKernel(dependency, context); | ||
} | ||
catch (HandlerException exception) | ||
if (!TryGetHandlerFromKernel(dependency, context, out var handler)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return false
case looks almost identical to the handler = null; return true
case below it.
There's a potentially a better factoring to indicate which of the two messages should be used?
6e82f54
to
1f642df
Compare
…wing first-chance exception
1f642df
to
93ce46c
Compare
{ | ||
handlers = kernel.GetHandlers(dependency.TargetItemType); | ||
} | ||
catch (HandlerException) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this a valid refactoring, we have to be sure that this method can't throw HandlerException
indirectly.
If you know that kernel.GetHandlers
would not throw HandlerException
, we can remove the try
. Same for the try
above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DefaultNamingSubSystem
doesn't appear to, however a custom INamingSubSystem
or IHandlersFilter
may. Sounds like a sensible way to write it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to merge. Could you update the changelog.
93ce46c
to
74b1999
Compare
CHANGELOG.md
Outdated
@@ -33,6 +33,9 @@ Breaking Changes: | |||
- Changed `IComponentModelBuilder.BuildModel` to use `Arguments` instead of `IDictionary` | |||
- Changed `ILazyComponentLoader.Load` to use `Arguments` instead of `IDictionary` | |||
|
|||
Bugfixes: | |||
- Fixed first-chance HandlerException for optional parameters (@jnm2, #450) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jnm2 You've added this inside the 5.0.0-beta001 section which has been released.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I'm sorry. Fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, thanks.
74b1999
to
82f86fb
Compare
Fixes #449.