EventSourceSink refactor #10768
EventSourceSink refactor #10768
Conversation
SimaTian
force-pushed
the
EventSourceSink-refactor-10245
branch
from
084aa69 to
01473dd
Compare
|
@dotnet-policy-service agree [company="Microsoft"] |
|
What is our approach to squashing? Do we squash everything to one commit before merge or is there some other sort of rule of thumb to follow please? |
There was a problem hiding this comment.
Nice reduction of code repetition!
I'd love this change to at the same time remove the #nullable disable to get stricter nullabvility checking and guarantees - as with the reduced code it should be easy to tackle. Or at least enable (#nullable enable) it for the refactored code
…g separate logging. removing null arguments, removing #nullable disable
We squash during merging (GH can do that for you). |
|
@dotnet-policy-service agree company="Microsoft" |
SimaTian
changed the title
| @@ -27,92 +25,92 @@ internal sealed class EventSourceSink : | |||
| /// <summary> | |||
| /// This event is raised to log a message. | |||
| /// </summary> | |||
| public event BuildMessageEventHandler MessageRaised; | |||
| public event BuildMessageEventHandler? MessageRaised; | |||
There was a problem hiding this comment.
why use the old event versus reactive extensions?
There was a problem hiding this comment.
IIRC this was discussed earlier on a different PR already.
- Rx.NET needs additional libs (not used accross VS or sdk) - which would require perf justification
- Introducing new paradigm to existing code base with diferent paradigm is net negative (there would be two paradigms or a very extensive refactoring of the whole codebase would be needed). In a new codebase it can be great
- This would be breaking to a fundamental public API (
IEventSource)
| break; | ||
| case BuildCheckEventArgs buildCheckEvent: | ||
| RaiseBuildCheckEvent(null, buildCheckEvent); | ||
| RaiseEvent(buildCheckEvent, args => BuildCheckEventRaised?.Invoke(null, args), RaiseAnyEvent); | ||
| break; | ||
|
|
||
| default: |
There was a problem hiding this comment.
consider a default action, at least a log entry
There was a problem hiding this comment.
It already has ErrorUtilities.ThrowInternalError("Unknown event args type.");
Fixes #10245
Context
EventSourceSink.cs
Changes Made
removed RaiseSomeEvent handlers and replaced them by generic one.
Testing
Notes