Add OpenXmlElement.IsValidChild method #1915
Conversation
mikeebowen
commented
Apr 4, 2025
mikeebowen
commented
- closes Add API to determine if a child is valid for a given element #1212
| } | ||
|
|
||
| [Fact] | ||
| public void IsValidChild_NullChild_ThrowsArgumentNullException() |
There was a problem hiding this comment.
should this throw or should it just return false?
There was a problem hiding this comment.
Updated to return false in latest commit
| throw new ArgumentNullException(nameof(element)); | ||
| } | ||
|
|
||
| return Metadata.Children.Elements.Any(el => el.Type.Name.Equals(element.Metadata.Type.Name)); |
There was a problem hiding this comment.
Wouldn't this only apply on OpenXmlCompositeElement? Should this be virtual and default to false and overriden on that type with this implementation?
There was a problem hiding this comment.
That's true, updated in latest commit
…CompositeElement, return false instead of throw when element child to test is null
| return false; | ||
| } | ||
|
|
||
| return Metadata.Children.Elements.Any(el => el.Type.Name.Equals(element.Metadata.Type.Name)); |
There was a problem hiding this comment.
| return Metadata.Children.Elements.Any(el => el.Type.Name.Equals(element.Metadata.Type.Name)); | |
| foreach(var child in Metadata.Children.Elements) | |
| { | |
| child.Type.Name.Equals(element.Metadata.Type.Name); | |
| } |
LINQ adds some allocations that we don't really need
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Globalization; | ||
| using System.IO; | ||
| using System.Linq; |
There was a problem hiding this comment.
| using System.Linq; |
if we remove the linq below
twsouthwick
left a comment
twsouthwick
left a comment
There was a problem hiding this comment.
LGTM just a thought about not using LINQ
| /// </summary> | ||
| /// <param name="element">The element to check.</param> | ||
| /// <returns>True if the specified element is a valid child; otherwise, false.</returns> | ||
| public virtual bool IsValidChild(OpenXmlElement element) |
There was a problem hiding this comment.
do we need this method on here? Would having it on OpenXmlCompositeElement be enough?
There was a problem hiding this comment.
I added the virtual method so it would be overridable, but I could make it virtual in OpenXmlCompositeElement
3 participants
