-
Notifications
You must be signed in to change notification settings - Fork 112
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.
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
added method to detect if a yaml fragment is a sequence #720
Conversation
[DataRow(@"_TestData/SchemaV3_0/Examples/Src/Components/MyHeaderComponent.pa.yaml", false)] | ||
[DataRow(@"_TestData/ValidYaml-CI/With-list-of-controls.pa.yaml", true)] | ||
[DataRow(@"_TestData/InvalidYaml/Dupliacte-Keys.pa.yaml", false)] | ||
public void IsSequenceCheckShouldWorkAsExpected(string path, bool expected) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please simplify this unit tests so it doesn't require external files for the baseline cases.
use targeted input strings for testing, don't use existing large files as these can change due to their purpose etc.
var results = deserializer.Deserialize<object[]>(reader); | ||
return results is not null; | ||
} | ||
catch (YamlException) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should not need to use exceptions as control-flow.
Another design option would be to deseralize into a custom type, register a converter for that type, and in the converter populate a property which has the 'result' of the detection.
e..g
private record SequenceDetectorResult
{
public required bool IsSequence { get; init; }
}
|
||
public object? ReadYaml(IParser parser, Type type) | ||
{ | ||
if (parser.Current is not SequenceStart) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when using the SequenceDetectorResult
pattern I mentioned above, this converter could just have this SequenceStart check, completely skip the contents, and then return the result value.
the benefit here is that there wouldn't be an exception thrown for code flow control.
I think this is a simple imlpementation that doesn't require calling the Deserializer infrastructure: public static bool DetectIsSequenceAtRoot(string yaml)
{
using var reader = new StringReader(yaml);
return DetectIsSequenceAtRoot(reader);
}
public static bool DetectIsSequenceAtRoot(StringReader reader)
{
var parser = new Parser(reader);
// Need to consume the StreamStart and DocumentStart events to get to the root sequence
parser.TryConsume<StreamStart>(out _);
parser.TryConsume<DocumentStart>(out _);
return parser.TryConsume<SequenceStart>(out _);
} |
Added a new
CheckIsSequence
method to detect if a yaml fragment is a sequence. It internally creates a new YamlDotNet deserializer that uses the custom type converterYamlSequenceTesterConverter
.This converter will simply check if the YAML fragment starts with the
SequenceStart
event and ends withSequenceEnd
.The following benchmark shows the results of a few deserialization methods:
IEnumerable<object>
using a basic YamlDotNet deserializerobject[]
using a basic YamlDotNet deserializerDictionary<string,object>[]
using a basic YamlDotNet deserializerCheckIsSequence
methodresults from
CheckIsSequence
are not so different from Direct deserialization toIEnumerable<object>
. We might decide to remove the customYamlSequenceTesterConverter
in the future and rely directly on the base library instead.