Corrected lack of XML.ElementInfo text and element child mixing #161
Conversation
ElementInfo: Added XmlNodeType Type Added IsText for validating ElementInfo as text element Added default ctor that sets Type to Element Added argument ctor to streamline construction Added TryContentAs methods for bool validation of content Added GetContentAs methods for default failed conversions of content Added TryAttributeAs methods Added GetAttributeAs methods Added AttributeAs methods for throwing exceptions Added TryAddChild for bool validation of add Added TrySetParent for bool validation of parent setting Added TrySetAttribute for bool validation attribute setting Added TryGetAttribute for bool validation and out of attribute string Added SetParent, and SetAttribute for method chaining Added GetAttribute for attribute string retrieval with default WriteToXML will write text elements with WriteString(Content) Added FromText for generating a text element from a string input Created ElementInfoStringExtensions for string handling Created ElementInfoListExtensions to take the place of ListExtensions ListExtensions is obsolete Created ElementInfoDictionaryExtensions for conversion for the attribute dictionary EventExecutor implements element and text mixing
|
I'd personally change the names and architecture a little -- specifically, I'd split |
I'm pretty sure that's a breaking change, can be considered for major version change, but I'm fairly sure that would violate semver to do otherwise and it actually could have capability to cause problems for mod devs if they used 5.1.0 and this is what anything other then 6.0.0 had in it. |
Changing the class architecture (i.e. adding I guess for that we could go for something DOMian here -- |
|
Something else: The |
This is kind of an intended output, perhaps it should say something like |
|
You sure we don't want, say, |
AsBoolean, AsFloat, and AsInt FormatExceptions describe both the content value and the key's value for dictionaries
| => ElementInfoListExtensions.TryGetElement(list, elementName, out info); | ||
| } | ||
|
|
||
| public static class ElementInfoDictionaryExtensions |
There was a problem hiding this comment.
Are these not the same extension methods that already exist in PathfinderAPI.Utils.DictionaryExtensions?
There was a problem hiding this comment.
Either I forgot or it was done before we had those, this stuff was around for a long time before I committed it.
| : throw new FormatException($"Value of '{valName}' is '{content}' which is not a float, e.g.: 1.0"); | ||
| } | ||
|
|
||
| public static class ElementInfoListExtensions |
There was a problem hiding this comment.
What is being done here? These methods are not extension methods in this class, yet the ones in the obsolete class got their API changed (it was changed from List to IEnumerable)
There was a problem hiding this comment.
This was a screwup, I'll fix it, would've been a breaking change if I don't.
| : throw new FormatException($"Value of '{Name}' is not a float, e.g.: 1.0"); | ||
| => Content.AsFloat(nameof(Content)); | ||
|
|
||
| public bool TryAttributeAsBoolean(string attribName, out bool result) |
There was a problem hiding this comment.
This naming scheme seems confusing, the behavior difference between GetAttributeAsX/AttributeAsX/TryAttributeAsX are not clear and I believe they would be prone to incorrect usage
There was a problem hiding this comment.
Yeah I probably should remove AttributeAs methods and instead add required and default behavior to GetAttributeAs like GetAttributeAsInt(string name, bool required = false, int def = default) (or could do GetAttributeAsInt(string name, int? def = null))
| return Children.Last() == info; | ||
| } | ||
|
|
||
| public bool TrySetParent(ElementInfo info) |
There was a problem hiding this comment.
I don't think this makes sense to be here, an element doesn't doesn't care about its parent, so why would you set it through the child element
There was a problem hiding this comment.
I suppose, was doing it for convenience but I suppose a child doesn't need to be aware of its parent's add success
| @@ -16,6 +19,20 @@ public class ElementInfo | |||
| public Dictionary<string, string> Attributes { get; set; } = new Dictionary<string, string>(); | |||
| public List<ElementInfo> Children { get; set; } = new List<ElementInfo>(); | |||
| public ulong NodeID { get; } = freeId++; | |||
| public XmlNodeType Type { get; set; } | |||
| public bool IsText => Type == XmlNodeType.Text; | |||
There was a problem hiding this comment.
This entire structure is confusing, why can a text element have attributes and children if they will never be serialized? This also makes adding text to an element two operations for a developer, since I have to both create a child element and also append to Content myself to keep everything up to date. These being out of sync really shouldn't be allowed.
ElementInfo:
Added XmlNodeType Type
Added IsText for validating ElementInfo as text element
Added default ctor that sets Type to Element
Added argument ctor to streamline construction
Added TryContentAs methods for bool validation of content
Added GetContentAs methods for default failed conversions of content
Added TryAttributeAs methods
Added GetAttributeAs methods
Added AttributeAs methods for throwing exceptions
Added TryAddChild for bool validation of add
Added TrySetParent for bool validation of parent setting
Added TrySetAttribute for bool validation attribute setting
Added TryGetAttribute for bool validation and out of attribute string
Added SetParent, and SetAttribute for method chaining
Added GetAttribute for attribute string retrieval with default
WriteToXML will write text elements with WriteString(Content)
Added FromText for generating a text element from a string input
Created ElementInfoStringExtensions for string handling
Created ElementInfoListExtensions to take the place of ListExtensions
ListExtensions is obsolete
Created ElementInfoDictionaryExtensions for conversion for the attribute dictionary
EventExecutor implements element and text mixing