-
Notifications
You must be signed in to change notification settings - Fork 2
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
Move IScopedNode from SDK #228
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
ac5996f
First try
marcovisserFurore bafae07
Moved IScopedNode from firely-net-sdk to here.
marcovisserFurore 475a97f
Merge branch 'develop' into feature/move-iscopednode-from-sdk
marcovisserFurore 10ac1c8
After review EK: avoid casting at Children() function, but do it at p…
marcovisserFurore File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
/* | ||
* Copyright (c) 2023, Firely (info@fire.ly) and contributors | ||
* See the file CONTRIBUTORS for details. | ||
* | ||
* This file is licensed under the BSD 3-Clause license | ||
* available at https://raw.githubusercontent.com/FirelyTeam/firely-net-sdk/master/LICENSE | ||
*/ | ||
|
||
#nullable enable | ||
|
||
using Hl7.Fhir.ElementModel; | ||
|
||
namespace Firely.Fhir.Validation | ||
{ | ||
/// <summary> | ||
/// An element within a tree of typed FHIR data with also a parent element. | ||
/// </summary> | ||
/// <remarks> | ||
/// This interface represents FHIR data as a tree of elements, including type information either present in | ||
/// the instance or derived from fully aware of the FHIR definitions and types | ||
/// </remarks> | ||
#pragma warning disable CS0618 // Type or member is obsolete | ||
internal interface IScopedNode : IBaseElementNavigator<IScopedNode> | ||
#pragma warning restore CS0618 // Type or member is obsolete | ||
{ | ||
/* | ||
* | ||
/// <summary> | ||
/// The parent node of this node, or null if this is the root node. | ||
/// </summary> | ||
IScopedNode? Parent { get; } // We don't need this probably. | ||
*/ | ||
} | ||
} | ||
|
||
#nullable restore |
57 changes: 57 additions & 0 deletions
57
src/Firely.Fhir.Validation/Support/IScopedNodeExtensions.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
/* | ||
* Copyright (c) 2023, Firely (info@fire.ly) and contributors | ||
* See the file CONTRIBUTORS for details. | ||
* | ||
* This file is licensed under the BSD 3-Clause license | ||
* available at https://raw.githubusercontent.com/FirelyTeam/firely-net-sdk/master/LICENSE | ||
*/ | ||
|
||
#nullable enable | ||
|
||
|
||
using Hl7.Fhir.ElementModel; | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
|
||
namespace Firely.Fhir.Validation | ||
{ | ||
internal static class IScopedNodeExtensions | ||
{ | ||
/// <summary> | ||
/// Converts a <see cref="IScopedNode"/> to a <see cref="ITypedElement"/>. | ||
/// </summary> | ||
/// <param name="node">An <see cref="IScopedNode"/> node</param> | ||
/// <returns>An implementation of <see cref="ITypedElement"/></returns> | ||
/// <remarks>Be careful when using this method, the returned <see cref="ITypedElement"/> does not implement | ||
/// the methods <see cref="ITypedElement.Location"/> and <see cref="ITypedElement.Definition"/>. | ||
/// </remarks> | ||
[Obsolete("WARNING! For internal API use only. Turning an IScopedNode into an ITypedElement will cause problems for" + | ||
"Location and Definitions. Those properties are not implemented using this method and can cause problems " + | ||
"elsewhere. Please don't use this method unless you know what you are doing.")] | ||
public static ITypedElement AsTypedElement(this IScopedNode node) => node switch | ||
{ | ||
TypedElementToIScopedNodeToAdapter adapter => adapter.ScopedNode, | ||
ITypedElement ite => ite, | ||
_ => new ScopedNodeToTypedElementAdapter(node) | ||
}; | ||
//node is ITypedElement ite ? ite : new ScopedNodeToTypedElementAdapter(node); | ||
|
||
public static ScopedNode ToScopedNode(this IScopedNode node) => node switch | ||
{ | ||
TypedElementToIScopedNodeToAdapter adapter => adapter.ScopedNode, | ||
_ => throw new ArgumentException("The node is not a TypedElementToIScopedNodeToAdapter") | ||
}; | ||
|
||
/// <summary> | ||
/// Returns the parent resource of this node, or null if this node is not part of a resource. | ||
/// </summary> | ||
/// <param name="nodes"></param> | ||
/// <param name="name"></param> | ||
/// <returns></returns> | ||
public static IEnumerable<IScopedNode> Children(this IEnumerable<IScopedNode> nodes, string? name = null) => | ||
nodes.SelectMany(n => n.Children(name)); | ||
} | ||
} | ||
|
||
#nullable restore |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
48 changes: 48 additions & 0 deletions
48
src/Firely.Fhir.Validation/Support/ScopedNodeToTypedElementAdapter.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
/* | ||
* Copyright (c) 2023, Firely (info@fire.ly) and contributors | ||
* See the file CONTRIBUTORS for details. | ||
* | ||
* This file is licensed under the BSD 3-Clause license | ||
* available at https://raw.githubusercontent.com/FirelyTeam/firely-net-sdk/master/LICENSE | ||
*/ | ||
|
||
#nullable enable | ||
|
||
using Hl7.Fhir.ElementModel; | ||
using Hl7.Fhir.Specification; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
|
||
namespace Firely.Fhir.Validation | ||
{ | ||
/// <summary> | ||
/// An adapter from <see cref="IScopedNode"/> to <see cref="ITypedElement"/>. | ||
/// </summary> | ||
/// <remarks>Be careful, this adapter does not implement the <see cref="ITypedElement.Location"/> and | ||
/// <see cref="ITypedElement.Definition"/> property. | ||
/// </remarks> | ||
internal class ScopedNodeToTypedElementAdapter : ITypedElement | ||
{ | ||
private readonly IScopedNode _adaptee; | ||
|
||
public ScopedNodeToTypedElementAdapter(IScopedNode adaptee) | ||
{ | ||
_adaptee = adaptee; | ||
} | ||
|
||
public string Location => throw new System.NotImplementedException(); | ||
|
||
public IElementDefinitionSummary Definition => throw new System.NotImplementedException(); | ||
|
||
public string Name => _adaptee.Name; | ||
|
||
public string InstanceType => _adaptee.InstanceType; | ||
|
||
public object Value => _adaptee.Value; | ||
|
||
public IEnumerable<ITypedElement> Children(string? name = null) => | ||
_adaptee.Children(name).Select(n => new ScopedNodeToTypedElementAdapter(n)); | ||
} | ||
} | ||
|
||
#nullable restore |
31 changes: 31 additions & 0 deletions
31
src/Firely.Fhir.Validation/Support/TypedElementToIScopedNodeToAdapter.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
using Hl7.Fhir.ElementModel; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
|
||
namespace Firely.Fhir.Validation | ||
{ | ||
internal class TypedElementToIScopedNodeToAdapter : IScopedNode | ||
{ | ||
private readonly ITypedElement _adaptee; | ||
|
||
public TypedElementToIScopedNodeToAdapter(ScopedNode adaptee) : this(adaptee as ITypedElement) | ||
{ | ||
} | ||
|
||
private TypedElementToIScopedNodeToAdapter(ITypedElement adaptee) | ||
{ | ||
_adaptee = adaptee; | ||
} | ||
|
||
public ScopedNode ScopedNode => (ScopedNode)_adaptee; // we know that this is always a ScopedNode | ||
|
||
public string Name => _adaptee.Name; | ||
|
||
public string InstanceType => _adaptee.InstanceType; | ||
|
||
public object Value => _adaptee.Value; | ||
|
||
IEnumerable<IScopedNode> IBaseElementNavigator<IScopedNode>.Children(string? name) => | ||
_adaptee.Children(name).Select(n => new TypedElementToIScopedNodeToAdapter(n)); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
_adaptee
is aScopedNode
, we don't need to wrap it I think, asScopedNode
already implementsITypedElement
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we do have to cast it, because the constructor of
TypedElementToIScopedNodeToAdapter
needs aScopedNode
as an argument. When I skip the cast, the constructor will not work anymore.Why not make the
_adaptee
of typeITypedElement
you would say? When we do that we have to do the cast at the propertypublic ScopedNode ScopedNode
instead. So I chose for the casting at theChildren()
function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I change the class a bit. The field
_adaptee
is now of typeITypedElement
and we have a private constructor that takes anITypedElement
. Doing so, we don't need the cast in children anymore. The cast moved to propertyScopedNode
.Wrapping is still needed because this adapter implements
IScopedNode
. ScopedNode implementsITypedElement
, but notIScopedNode
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I misread your comment. This about the
ScopedNodeToTypedElementAdapter
. I was talking aboutTypedElementToIScopedNodeToAdapter
.About
ScopedNodeToTypedElementAdapter
:This adapter is only used by the extension method
AsTypedElement
. Looking at the implementation of this method we see: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!