-
Notifications
You must be signed in to change notification settings - Fork 153
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
Consider opening more of DiagnosticResult surface #661
Comments
Preventing breaking changes while being flexible to change its internals is obviously crucial for any reusable library, as it is for Simple Injector. The strategy has always been to keep things internal until there are clear use cases for external use. Can you therefore describe what your use case is? How are you trying to extend the library? I might want to add such example in the CodeSamples project. This is the place I show case user extensions and it helps me track down how changes might break consumers. |
We're using conditional registrations to register dependencies for specific "target" types (testing for context.Consumer.ImplementationType). Unfortunately, recently we've been hit by an nasty error that stemmed from the fact that a wrong registration was made -- the target service depended on an interface, and the developer conditionally registered an implementation of that interface as a service type, which lead to another unconditional (fallback) registration of the same dependency to be used. container.Verify(VerificationOption.VerifyAndDiagnose);
var results = new List<DiagnosticResult>();
PrepareAndAnalyze(container, results);
results.AddRangeHinted(Analyzer.Analyze(container));
if (results.Count != 0)
{
throw new DiagnosticVerificationException(results);
} ... plugging into this mechanism would be useful. |
Your use case might actually be an interesting feature to include out of the box. However, in that case, it would likely be presented as an info message, rather than a warning (because I rather not throw on this). And even though interesting to add, that will not help you at this point, as it will take some time before such feature can released (it will unlikely make the v4.5 release). Either way, I added a new issue (#662) for this. Feel free to add the code you use internally to speed things up. Making the As you already noticed in your last sentence, there is no special need to integrate at this level into Simple Injector by deriving from Because of that, although I understand your request, I prefer keeping things sealed and internal for now, as that makes it much easier to add new features without having to prevent breaking changes. For instance, feature #553 is something I'm currently working on, that benefits from having a lot of internal, non-extendible, parts. The feature is hard enough by itself, while being almost impossible to achieve if it had more externally exposed (and unchangeable) parts. |
Thank you for the detailed answer, I really appreciate it. I think I agree with everything you said here, this case could be handled by the container itself, and outside of this scope there is little value of adding new DiagnosticResult type, so exposing internals isn't really needed. |
Currently, DiagnosticResult ctor and several of its properties are internal, which prohibits creation of new DiagnosticResult types. I certainly understand that exposing more of internals potentially increases the amount of future breaking changes, but DiagnosticResult looks like a value object, so the risk seems lower in this case.
Please consider making the DiagnosticResult protected, along with making the Value property public. This will allow inheriting from DiagnosticResult in "user" code and allow custom diagnostic results.
The text was updated successfully, but these errors were encountered: