Move IScopedNode from SDK #228
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| public object Value => _adaptee.Value; | ||
|
|
||
| public IEnumerable<ITypedElement> Children(string? name = null) => | ||
| _adaptee.Children(name).Select(n => new ScopedNodeToTypedElementAdapter(n)); |
There was a problem hiding this comment.
If _adaptee is a ScopedNode, we don't need to wrap it I think, as ScopedNode already implements ITypedElement?
There was a problem hiding this comment.
Yes we do have to cast it, because the constructor of TypedElementToIScopedNodeToAdapter needs a ScopedNode as an argument. When I skip the cast, the constructor will not work anymore.
Why not make the _adaptee of type ITypedElement you would say? When we do that we have to do the cast at the property public ScopedNode ScopedNode instead. So I chose for the casting at the Children() function.
There was a problem hiding this comment.
But it is not a cast, we are creating lots of new objects, which I know by testing both increases GC load and slows down validation - so I want to make sure we really need this!
There was a problem hiding this comment.
Ok, I change the class a bit. The field _adaptee is now of type ITypedElement and we have a private constructor that takes an ITypedElement. Doing so, we don't need the cast in children anymore. The cast moved to property ScopedNode.
Wrapping is still needed because this adapter implements IScopedNode. ScopedNode implements ITypedElement, but not IScopedNode.
There was a problem hiding this comment.
I misread your comment. This about the ScopedNodeToTypedElementAdapter. I was talking about TypedElementToIScopedNodeToAdapter.
About ScopedNodeToTypedElementAdapter:
This adapter is only used by the extension method AsTypedElement. Looking at the implementation of this method we see:
public static ITypedElement AsTypedElement(this IScopedNode node) => node switch
{
TypedElementToIScopedNodeToAdapter adapter => adapter.ScopedNode,
ITypedElement ite => ite,
_ => new ScopedNodeToTypedElementAdapter(node)
};The first 2 cases already bypass the adapter. Only in rare cases the adapter is used. In fact, I ran all the tests and the adapter is never used!
…operty ScopedNode. This is better for the performance.
Move the interface IScopedNod from the firely-net-sdk repo.