[Design] Add overloads for IsEnabled #65
Conversation
|
@jbagga, |
| @@ -54,6 +69,25 @@ private static Listener EnlistTarget(object target, Func<string, bool> isEnabled | |||
| return listener; | |||
| } | |||
|
|
|||
| private static Listener EnlistTarget(object target, Func<string, object, object, bool> isEnabled) | |||
There was a problem hiding this comment.
You can turn a Func<string, bool> into a Func<string, object, object, bool> so then you only need one 'real' code path.
Func<string, bool> original = ...;
Func<string, object, object, bool> adapter = (a, b, c) => original(a);
| (_listener.IsEnabled == null || _listener.IsEnabled(diagnosticName, null, null)); | ||
| } | ||
|
|
||
| public bool IsEnabled(string diagnosticName, object arg1, object arg2 = null) |
There was a problem hiding this comment.
This is a little bit of a wonky signature. Where does it come from?
|
|
||
| return listener; | ||
| } | ||
|
|
||
| public bool IsEnabled(string diagnosticName) |
There was a problem hiding this comment.
Make this one call the longer o/l
| public Listener(object target, Func<string, bool> isEnabled) | ||
| { | ||
| Target = target; | ||
| IsEnabled = (isEnabled == null) ? (Func<string, object, object, bool>) null : (a, b, c) => isEnabled(a); |
There was a problem hiding this comment.
Yeah you figured it out 👍 - flow this up one level of abstraction and that will dedupe some code
| @@ -79,7 +125,7 @@ public void Write(string diagnosticName, object parameters) | |||
| return; | |||
| } | |||
|
|
|||
| if (_listener.IsEnabled != null && !_listener.IsEnabled(diagnosticName)) | |||
| if (_listener.IsEnabled != null && !_listener.IsEnabled(diagnosticName, null, null)) | |||
There was a problem hiding this comment.
So if we just always call this with null, null then we're not really supporting it. Where are we supposed to get these values from?
There was a problem hiding this comment.
I am not entirely sure. I think it can be any context we want to associate with the events. https://github.com/dotnet/corefx/issues/15984#issuecomment-278692599 I was hoping to hear your thoughts on this.
There was a problem hiding this comment.
Do you think this can now be a part of the event context instead of a class?
|
Updated. Spoke to @rynowak offline and made some behavior changes to the cc @Eilon |
| public DiagnosticSourceAdapter( | ||
| object target, | ||
| Func<string, bool> isEnabled, | ||
| IDiagnosticSourceMethodAdapter methodAdapter) | ||
| { | ||
| _methodAdapter = methodAdapter; | ||
| _listener = EnlistTarget(target, (isEnabled == null) ? (Func<string, object, object, bool>)null : (a, b, c) => isEnabled(a)); | ||
| } |
There was a problem hiding this comment.
Can you make this constructor chain to the one at L42?
| IsEnabled = (isEnabled == null) ? (Func<string, object, object, bool>) null : (a, b, c) => isEnabled(a); | ||
|
|
||
| Subscriptions = new Dictionary<string, Subscription>(StringComparer.Ordinal); | ||
| } |
There was a problem hiding this comment.
Please delete this constructor, and keep the one at L154.
| @@ -33,10 +33,13 @@ public class OneTarget | |||
| { | |||
| public int OneCallCount { get; private set; } | |||
|
|
|||
| public object TargetInfo { get; private set; } | |||
| Assert.Equal("Target info", arg1); | ||
| callCount++; | ||
| return true; | ||
| }; |
There was a problem hiding this comment.
Add a test where you register a Func<string, bool> but call `IsEnabled(string, object, object)
|
@rynowak Updated |
Addresses #61
Are there test cases I should add? I don't have enough context yet.
cc @rynowak