Repository navigation
Rename callBase to implementation - #109
Conversation
In preparation for adding decorator support, it's becoming clearer that that callBase delegate is actually the implementation of a method, be it from a call to the base implementation in a virtual method or the invocation of a target object provided as the instance being decorated for an interface avatar. In order to avoid having two distinct concepts for the same idea (i.e. in IMethodInvocation we have a CreateCallBaseReturn), rename things consistently so we only talk about the ultimate implementation of the method, so, HasImplementation and CreateInvokeReturn (to invoke that implementation). When we introduce decorators, we will not need to add additional public API members in the pipeline for that. Fixes #108.
There was a problem hiding this comment.
A few minor things I noticed while taking a coarse glance at this commit. I might possibly misunderstand the meanings of these terms.
| bool SupportsCallBase { get; } | ||
| /// <remarks> | ||
| /// For a class-based avatar, this would be <see langword="true"/> for virtual | ||
| /// methods, for an interface-based avatar, it would be <see langword="true"/> |
There was a problem hiding this comment.
This distinction between class and interface avatar behavior appears to be incomplete: What about interface methods with a default implementation? Will interfaces with default implementations be modelled as decorators?
There was a problem hiding this comment.
Hm.. how do those even work at the IL level? Right now, I don't even think the currently generated avatars consider this scenario... Could you file a new issue? Worth exploring and supporting! Thanks for bringing it up :)
| /// <remarks> | ||
| /// For a class-based avatar, this would be <see langword="true"/> for virtual | ||
| /// methods, for an interface-based avatar, it would be <see langword="true"/> | ||
| /// if a target implementation instance was provided (i.e. as a decorator pattern). |
There was a problem hiding this comment.
I think I understand why you want to get rid of the term "call base", but with the new term "implementation" it appears there's now some overlap with "target", which can be seen here: "target implementation". It might pay off to be extra careful about when you use the word "target"... shouldn't it be reserved exclusively for whatever the Target property refers to, and be avoided whenever talking about implementation? Or do you mean "implementation on the target instance"?
There was a problem hiding this comment.
Good point. Perhaps renaming Target property to Instance is better? It's basically the instance on which the method invocation was performed...
| /// base implementation. In other words, it's either abstract or a member of an interface.</exception> | ||
| /// <exception cref="NotSupportedException">The call base implementation attempted to get the | ||
| /// base implementation. In other words, it's either abstract or a member of an interface (with | ||
| /// no target implementation).</exception> |
There was a problem hiding this comment.
Same issue here, is a default implementation for an interface referred to as a "target"? I suspect you wrote this with only decorator target instances in mind.
| /// <param name="callBase">Delegate to invoke the base method implementation for virtual methods.</param> | ||
| public MethodInvocation(object target, MethodBase method, ExecuteHandler callBase) | ||
| : this(target, method, callBase, new ArgumentCollection()) | ||
| /// <param name="implementation">Delegate to invoke the method implementation.</param> |
There was a problem hiding this comment.
Is this documentation comment for implementation intentionally different from e.g. MethodInvocation.Create? There you kept the words "base [method implementation] for virtual methods", you dropped those here.
|
|
||
| if (Behaviors.Count == 0) | ||
| return CallBaseOrThrow(invocation); | ||
| return InvokeTargetOrThrow(invocation); |
There was a problem hiding this comment.
InvokeTarget... or InvokeImplementation... (or InvokeImplementationOnTarget...)?
|
I'll stop setting the auto merge and wait for your comments! |
|
@kzu whatever you prefer. I'm happy to do reviews before, or continue to be a somewhat pedantic commenter on already merged code. :-) I may not spot every PR in the GitHub activity feed, so you may end up waiting too long at times. Feel free to ping me if you'd like a second opinion on something specific. |
In preparation for adding decorator support, it's becoming clearer that that callBase delegate is actually the implementation of a method, be it from a call to the base implementation in a virtual method or the invocation of a target object provided as the instance being decorated for an interface avatar.
In order to avoid having two distinct concepts for the same idea (i.e. in IMethodInvocation we have a CreateCallBaseReturn), rename things consistently so we only talk about the ultimate implementation of the method, so, HasImplementation and CreateInvokeReturn (to invoke that implementation).
When we introduce decorators, we will not need to add additional public API members in the pipeline for that.
Fixes #108.