feat(objects): cnx 849 finalized DataObjects #185
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #185 +/- ##
===========================================
+ Coverage 53.68% 66.24% +12.55%
===========================================
Files 233 251 +18
Lines 9367 10016 +649
Branches 1065 1065
===========================================
+ Hits 5029 6635 +1606
+ Misses 4084 3035 -1049
- Partials 254 346 +92 ☔ View full report in Codecov by Sentry. |
src/Speckle.Objects/Interfaces.cs
Outdated
| Dictionary<string, object?> properties { get; } | ||
| } | ||
|
|
||
| public interface IDataObject : ISpeckleObject, IProperties |
There was a problem hiding this comment.
Did we want IDataObject : IDisplayValue?
Is there any value still in IDisplayValue? or shall we kill it
There was a problem hiding this comment.
Ah, I see your comment (tho you could have made it implement IDisplayValue<IReadOnlyList<Base>?>)
But I'd like to discuss both the nullability, and existence of IDisplayValue
There was a problem hiding this comment.
Agreed with Claire IDataObject : IDisplayValue with the intention of maybe cleaning up and removing this interface in future
src/Speckle.Objects/Interfaces.cs
Outdated
| /// Should be simple geometry types: Point, Line, Polyline, and Mesh. | ||
| /// Null indicates a non-displayable data object. | ||
| /// </remarks> | ||
| IReadOnlyList<Base>? displayValue { get; } |
There was a problem hiding this comment.
having this nullable...
is there a semantic difference between null and an empty list?
Thinking where we might be sending empty lists
- Navisworks groups, Blender empties, Empty Sketchup components/groups?
If not, then I'd suggest we stick with empty lists.
Otherwise all consumers will have to check for null or empty, and presumably handle them both exactly the same way...
Opens opportunity for us to mix and match, and risk having some consumers forget about one of the cases
There was a problem hiding this comment.
Agreed with Claire we should remove the ? from the displayValue
| { | ||
| public required string name { get; set; } | ||
|
|
||
| public required List<Base> displayValue { get; set; } |
There was a problem hiding this comment.
[DetachedProperty] should be on all displayValue properties (also missing from several other classes)
| public required string units { get; set; } | ||
|
|
||
| IReadOnlyList<ICivilObject> ICivilObject.elements => elements; | ||
| IReadOnlyList<Base> ICivilObject.elements => elements; |
There was a problem hiding this comment.
We should double check that the serializer doesn't accidently try and grab this property @adamhathcock
We can set these as [JsonIgnore], but I'd like to understand if there's something else that's nicely guaranteeing that the "hidden" elements prop (explicit interface implementation) is not ever prefered over the "real" .elements property.
Finalizes interfaces for
IDataObject:propertiesdictionarynote: the IDataObject interface does not implement
IDisplayValuedue to nullability atm. should be evaluated.