-
-
Notifications
You must be signed in to change notification settings - Fork 496
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
Internal exceptions being thrown #555
Comments
@daveaglick I would want us to do the right thing here, so a PR would be appreciated! |
@daveaglick you're still working on this one? (I mean, there's still a day left in your self-chosen timeframe, so I wouldn't want to put any pressure on you. 😬) |
Lol. I like to live on the edge. Nothing like a little pressure! |
I was all set to implement this as described (only two months later, nbd!) but after digging in, realized that maybe this is just a documentation issue. Consider this code I found in var value = resolver.Resolve(parameter.ParameterType);
if (value == null)
{
throw CommandRuntimeException.CouldNotResolveType(parameter.ParameterType);
} That strongly suggests that the expected behavior of try
{
if (resolver.Resolve(settingsType) is CommandSettings settings)
{
return settings;
}
}
catch
{
// ignored
} That code has a try/catch guard, suggesting So my question is this: should I had assumed the former and implemented my own |
and does not throw on unresolvable types. Also changed the TypeResolverAdapter to adhere to those expectations and removed the now no longer needed try-catch from CommandPropertyBinder.
as a very simple test harness to test implementations of ITypeRegistrar / ITypeResolver
as a very simple test harness to test implementations of ITypeRegistrar / ITypeResolver
and does not throw on unresolvable types. Also changed the TypeResolverAdapter to adhere to those expectations and removed the now no longer needed try-catch from CommandPropertyBinder.
as a very simple test harness to test implementations of ITypeRegistrar / ITypeResolver
@daveaglick not throwing would be the expectation. I tried to make this clearer in #620. Closed in #620. |
Looks great, thanks! |
I updated Spectre.Console and started seeing handled exceptions being thrown, got curious, and traced it back to PR #376. The problem is that while the new
try/catch
introduced in this PR around theresolver.Resolve(settingsType)
call works, it ends up creating an expected control flow where throwing is "normal". That's fine as far as it goes, but it does result in unnecessary exceptions under the hood which will both impact performance (though likely not in any critical way) and trigger exception filters when debugging. And it feels icky :)The reason I didn't see these exceptions before the update was because
TypeRegistrarExtensions.RegisterDependencies()
was registering the settings types in my container for me, so when the old code asked the container for them, they were already in there. That code was removed in this PR so the container doesn't have the settings type registered any more.It's unclear whether this is a "problem" that needs solving. I generally don't love throwing exceptions during expected flows, even if I'm catching them. If there's an interest in changing the behavior the best thing I can think of is to introduce an
ITypeResolver.TryResolve()
that can return a null value for unregistered types without throwing (or alternatively use abool
/out
pattern). Then use that to resolve the settings type without atry
/catch
and fallback to direct instantiation if it returns null.Would you be interested in a PR for this or should the behavior be considered by design?
The text was updated successfully, but these errors were encountered: