Skip to content
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.

Sign up for GitHub

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

Corrected lack of XML.ElementInfo text and element child mixing #161

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Corrected lack of XML.ElementInfo text and element child mixing #161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
62dc3c4
Corrected lack of XML.ElementInfo text and element child mixing
Spartan322 Apr 25, 2022
c8112b8
Prevent null attribute dictionary reading from throwing an NRE
Spartan322 Apr 25, 2022
14f35dd
Make type parse exceptions more useful
Spartan322 Apr 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
245 changes: 212 additions & 33 deletions PathfinderAPI/Util/XML/ElementInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
using System.Text;
using System.Linq;
using System;
using System.Collections.Generic;
using System.Text;
using System.Xml;

namespace Pathfinder.Util.XML;
Expand All @@ -16,6 +19,20 @@ public string Content {
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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backwards compatibility, this was addressed with here. It obviously doesn't make sense but without a refactor which would introduce breaking changes, which is addressed with #163, there is no safe way for mods to rely on the API's semver being true.


public ElementInfo() { Type = XmlNodeType.Element; }

public ElementInfo(string name, string content = null, Dictionary<string, string> attributes = null, List<ElementInfo> children = null, ElementInfo parent = null)
: this()
{
Name = name;
if(content != null) Content = content;
Attributes = attributes ?? new Dictionary<string, string>();
Children = children ?? new List<ElementInfo>();
Parent = parent;
}

public override string ToString()
{
Expand All @@ -28,60 +45,222 @@ public override string ToString()
{
WriteToXML(writer);
}

return builder.Replace("\t", " ").ToString();
}

public bool TryContentAsBoolean(out bool result)
=> Content.TryAsBoolean(out result);

public bool TryContentAsInt(out int result)
=> Content.TryAsInt(out result);

public bool TryContentAsFloat(out float result)
=> Content.TryAsFloat(out result);

public bool GetContentAsBoolean(bool defaultVal = default)
=> TryContentAsBoolean(out var result) ? result : defaultVal;

public int GetContentAsInt(int defaultVal = default)
=> TryContentAsInt(out var result) ? result : defaultVal;

public float GetContentAsFloat(float defaultVal = default)
=> TryContentAsFloat(out var result) ? result : defaultVal;

public bool ContentAsBoolean()
=> bool.TryParse(Content, out var value)
? value
: throw new FormatException($"Value of '{Name}' is not true or false");
=> Content.AsBoolean(nameof(Content));

public int ContentAsInt()
=> int.TryParse(Content, out var value)
? value
: throw new FormatException($"Value of '{Name}' is not an integer, e.g.: 0, 1, 2");

=> Content.AsInt(nameof(Content));

public float ContentAsFloat()
=> float.TryParse(Content, out var value)
? value
: 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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@Spartan322 Spartan322 May 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

=> Attributes.TryAsBoolean(attribName, out result);

public bool TryAttributeAsInt(string attribName, out int result)
=> Attributes.TryAsInt(attribName, out result);

public bool TryAttributeAsFloat(string attribName, out float result)
=> Attributes.TryAsFloat(attribName, out result);

public bool GetAttributeAsBoolean(string attribName, bool defaultVal = default)
=> TryAttributeAsBoolean(attribName, out var result) ? result : defaultVal;

public int GetAttributeAsInt(string attribName, int defaultVal = default)
=> TryAttributeAsInt(attribName, out var result) ? result : defaultVal;

public float GetAttributeAsFloat(string attribName, float defaultVal = default)
=> TryAttributeAsFloat(attribName, out var result) ? result : defaultVal;

public bool AttributeAsBoolean(string attribName)
=> Attributes.AsBoolean(attribName, $"{nameof(Attributes)}");

public int AttributeAsInt(string attribName)
=> Attributes.AsInt(attribName, $"{nameof(Attributes)}");

public float AttributeAsFloat(string attribName)
=> Attributes.AsFloat(attribName, $"{nameof(Attributes)}");

public bool TryAddChild(ElementInfo info)
{
if(Children == null) return false;
if(info.Parent != null)
info.Parent.Children.Remove(info);
Children.Add(info);
info.Parent = this;
return Children.Last() == info;
}

public bool TrySetParent(ElementInfo info)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose, was doing it for convenience but I suppose a child doesn't need to be aware of its parent's add success

{
return info.TryAddChild(this);
}

public bool TrySetAttribute(string key, string value)
{
if(Attributes == null) return false;
Attributes[key] = value;
return Attributes[key] == value;
}

public bool TryGetAttribute(string key, ref string value)
{
if(!Attributes?.TryGetValue(key, out value) ?? true) return false;
return true;
}

public ElementInfo AddChild(ElementInfo info)
{
TryAddChild(info);
return this;
}

public ElementInfo SetParent(ElementInfo info)
{
TrySetParent(info);
return this;
}

public ElementInfo SetAttribute(string key, string value)
{
TrySetAttribute(key, value);
return this;
}

public string GetAttribute(string key, string defaultValue = null)
{
TryGetAttribute(key, ref defaultValue);
return defaultValue;
}

public void WriteToXML(XmlWriter writer)
{
if(IsText)
{
writer.WriteString(Content);
return;
}
writer.WriteStartElement(Name, "");
foreach (var attr in Attributes)
writer.WriteAttributeString(attr.Key, attr.Value);
if (Content == null)
{
foreach (var child in Children)
child.WriteToXML(writer);
}
else
{
writer.WriteValue(Content);
}
foreach (var child in Children)
child.WriteToXML(writer);
writer.WriteEndElement();
}

public static ElementInfo FromText(string input)
=> new ElementInfo
{
Content = input,
Type = XmlNodeType.Text,
Attributes = null,
Children = null
};
}

public static class ListExtensions
public static class ElementInfoStringExtensions
{
public static ElementInfo GetElement(this List<ElementInfo> list, string elementName)
{
foreach (var possibleInfo in list)
{
if (possibleInfo.Name == elementName)
{
return possibleInfo;
}
}
public static bool TryAsBoolean(this string content, out bool result)
=> bool.TryParse(content, out result);
public static bool TryAsInt(this string content, out int result)
=> int.TryParse(content, out result);
public static bool TryAsFloat(this string content, out float result)
=> float.TryParse(content, out result);

return null;
public static bool AsBooleanSafe(this string content, bool defaultVal = default)
=> content.TryAsBoolean(out var value)
? value
: defaultVal;
public static int AsIntSafe(this string content, int defaultVal = default)
=> content.TryAsInt(out var value)
? value
: defaultVal;
public static float AsFloatSafe(this string content, float defaultVal = default)
=> content.TryAsFloat(out var value)
? value
: defaultVal;

public static bool AsBoolean(this string content, string valName = "content")
=> content.TryAsBoolean(out var value)
? value
: throw new FormatException($"Value of '{valName}' is '{content}' which is not true or false");
public static int AsInt(this string content, string valName = "content")
=> content.TryAsInt(out var value)
? value
: throw new FormatException($"Value of '{valName}' is '{content}' which is not an integer, e.g.: 0, 1, 2");
public static float AsFloat(this string content, string valName = "content")
=> content.TryAsFloat(out var value)
? value
: throw new FormatException($"Value of '{valName}' is '{content}' which is not a float, e.g.: 1.0");
}

public static class ElementInfoListExtensions
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a screwup, I'll fix it, would've been a breaking change if I don't.

{
public static ElementInfo GetElement(IEnumerable<ElementInfo> list, string elementName)
{
return list.FirstOrDefault(e => e.Name == elementName);
}
public static bool TryGetElement(this List<ElementInfo> list, string elementName, out ElementInfo info)
public static bool TryGetElement(IEnumerable<ElementInfo> list, string elementName, out ElementInfo info)
{
info = GetElement(list, elementName);
return info != null;
}
}

[Obsolete("Use ElementInfoListExtensions")]
public static class ListExtensions
{
public static ElementInfo GetElement(this IEnumerable<ElementInfo> list, string elementName)
=> ElementInfoListExtensions.GetElement(list, elementName);
public static bool TryGetElement(this IEnumerable<ElementInfo> list, string elementName, out ElementInfo info)
=> ElementInfoListExtensions.TryGetElement(list, elementName, out info);
}

public static class ElementInfoDictionaryExtensions
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these not the same extension methods that already exist in PathfinderAPI.Utils.DictionaryExtensions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either I forgot or it was done before we had those, this stuff was around for a long time before I committed it.

{
public static bool TryAsBoolean(this IDictionary<string, string> attribute, string key, out bool result)
{
result = default;
if(attribute == null) return false;
return attribute.TryGetValue(key, out var str) ? str.TryAsBoolean(out result) : false;
}
public static bool TryAsInt(this IDictionary<string, string> attribute, string key, out int result)
{
result = default;
if(attribute == null) return false;
return attribute.TryGetValue(key, out var str) ? str.TryAsInt(out result) : false;
}
public static bool TryAsFloat(this IDictionary<string, string> attribute, string key, out float result)
{
result = default;
if(attribute == null) return false;
return attribute.TryGetValue(key, out var str) ? str.TryAsFloat(out result) : false;
}
public static bool AsBoolean(this IDictionary<string, string> attribute, string key, string dictName = "attribute")
=> attribute[key].AsBoolean($"{dictName}[{key}]");
public static int AsInt(this IDictionary<string, string> attribute, string key, string dictName = "attribute")
=> attribute[key].AsInt($"{dictName}[{key}]");
public static float AsFloat(this IDictionary<string, string> attribute, string key, string dictName = "attribute")
=> attribute[key].AsFloat($"{dictName}[{key}]");
}
10 changes: 2 additions & 8 deletions PathfinderAPI/Util/XML/EventExecutor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,14 +193,7 @@ protected override void ReadElement(Dictionary<string, string> attributes)
else
{
var topElement = currentElementStack.Peek();
var element = new ElementInfo()
{
Name = Reader.Name,
Attributes = attributes,
Parent = topElement,
Content = null
};
topElement.Children.Add(element);
var element = new ElementInfo(Reader.Name, attributes: attributes).SetParent(topElement);
currentElementStack.Push(element);
}
}
Expand Down Expand Up @@ -242,6 +235,7 @@ protected override void ReadText()
topElement.Content = Reader.Value;
else
topElement.Content += "\n" + Reader.Value;
topElement.AddChild(ElementInfo.FromText(Reader.Value));
}
}

Expand Down