Set DefaultEncodingId in DataTypeDefinition, was always null NodeId #1523
Conversation
mregen
commented
Sep 23, 2021
mregen
commented
- DefaultEncodingId is always set to binary encoding id
- the default encoding id is derived for the encodeable factory, since the typetree does not contain sufficent information to find the proper binary encoding id
- add a few helper functions in core for an upcoming PR that was used for testing with custom types, will simplify to add more nodemanagers to servers with custom types
| @@ -75,6 +72,12 @@ public interface IEncodeableFactory | |||
| /// <param name="assembly">The assembly containing the types to add to the factory</param> | |||
| void AddEncodeableTypes(Assembly assembly); | |||
|
|
|||
| /// <summary> | |||
| /// Adds an enumerable of extension types to the factory. | |||
Codecov Report
@@ Coverage Diff @@
## master #1523 +/- ##
==========================================
+ Coverage 52.96% 53.01% +0.05%
==========================================
Files 307 307
Lines 58376 58387 +11
==========================================
+ Hits 30919 30956 +37
+ Misses 27457 27431 -26
Continue to review full report at Codecov.
|
|
This pull request introduces 2 alerts when merging 9e6454c into 0d34b7d - view on LGTM.com new alerts:
|
| if (ServiceResult.IsGood(result)) | ||
| { | ||
| if (dataTypeDefinition?.Body is StructureDefinition structureType && | ||
| structureType.DefaultEncodingId.IsNullNodeId) |
There was a problem hiding this comment.
Is this piece of code covering enum datatypes as well? or they don't need special handling?
There was a problem hiding this comment.
good point, EnumDefinition does not have a DefaultEncodingId as a member, I wonder why it is only needed for struct... I believe enum are encoded as int anyway, so it may not even matter
|
sorry, I found a better way to add the setter code to the StructureDefinition, so it could be used for other means like metadataframe |
|
This pull request introduces 3 alerts when merging 2ed6867 into 0d34b7d - view on LGTM.com new alerts:
|
|
This pull request introduces 2 alerts when merging 857bf74 into 0d34b7d - view on LGTM.com new alerts:
|
