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

Consider opening more of DiagnosticResult surface #661

Closed
onyxmaster opened this issue Feb 12, 2019 · 4 comments
Closed

Consider opening more of DiagnosticResult surface #661

onyxmaster opened this issue Feb 12, 2019 · 4 comments

Comments

@onyxmaster
Copy link

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.

@dotnetjunkie
Copy link
Collaborator

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.

@onyxmaster
Copy link
Author

onyxmaster commented Feb 12, 2019

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.
This error could be prevented if we had a post-Verify (since we always Verify+Analyze on startup) analysis step that tested which conditional registrations were not used, adding them to a list of diagnostic results produced by Analyzer.Analyze.
Since we actually do something like:

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.
Of course, there is nothing that prevents us from wrapping diagnostic warnings in our type and throwing another exception, but, well, it's up to you.

@dotnetjunkie
Copy link
Collaborator

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 DiagnosticResult constructor public or protected, however, does cause other design challenges. There is currently a one-to-one mapping between DiagnosticResult derivatives and the DiagnosticType enumeration. Since you are not able to extend that enumeration, adding new sub types means creating a DiagnosticResult with an unknown (invalid) DiagnosticType value.

As you already noticed in your last sentence, there is no special need to integrate at this level into Simple Injector by deriving from DiagnosticResult, as you can simply "wrap diagnostic warnings in your type and throw another exception". This would be different if Simple Injector could internally make use of your DiagnosticResult and do some analysis on it, but that's something that needs integration at a much deeper level, while in most cases not needed as well.

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.

@onyxmaster
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants