Skip to content

[DotNet] Fix generation of classes that accept optional lists as constructor arguments #249

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

Merged
mpkorstanje merged 4 commits into from
Sep 12, 2024
Merged

[DotNet] Fix generation of classes that accept optional lists as constructor arguments #249

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bf1407e
[DotNet] Fix generation of classes that accept optional lists as cons…
clrudolphi Sep 9, 2024
fc7edd5
Modified for Required Lists
clrudolphi Sep 9, 2024
b099347
Fix gen of classes that accept optional lists as constructor args
clrudolphi Sep 12, 2024
fc7ee47
Merge branch 'main' into DotNet_FixHandlingOfNullConstructorArguments…
mpkorstanje Sep 12, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/)
and this project adheres to [Semantic Versioning](http://semver.org/).

## [Unreleased]
- [Dotnet] Fixed code generation for types that accept List<T> as parameters. Constructors were not properly handling null input. [clrudolphi]
Copy link
Contributor

@mpkorstanje mpkorstanje Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Spotted this after merging. Changes should be listed as one of Added, Changed, ect. I'll fix it on main.


## [26.0.0] - 2024-08-15
### Added
Expand Down
2 changes: 1 addition & 1 deletion codegen/templates/dotnet.dotnet.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public sealed class <%= class_name(key) %>
RequireNonNull<<%= type_for(class_name(key), property_name, property) -%>>(<%= property_name %>, "<%= capitalize(property_name) %>", "<%= class_name(key) %>.<%= capitalize(property_name) %> cannot be null");
<%- end -%>
<%- if property['items'] -%>
this.<%= capitalize(property_name) %> = new <%= type_for(class_name(key), property_name, property) -%>(<%= property_name %>);
this.<%= capitalize(property_name) %> = <%= property_name %> == null ? new <%= type_for(class_name(key), property_name, property) -%>() : new <%= type_for(class_name(key), property_name, property) -%>(<%= property_name %>);
<%- else -%>
this.<%= capitalize(property_name) %> = <%= property_name %>;
<%- end -%>
Expand Down
Binary file added dotnet/.vs/ProjectEvaluation/cucumber.messages.strings.v8.bin
View file
Open in desktop
Binary file not shown.
10 changes: 10 additions & 0 deletions dotnet/Cucumber.Messages.Specs/BasicMessageSerializationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,15 @@ public void SerializesAnEnvelopeToNDJSONCorrectly()
Assert.Equal(expectedStepDefinition, reconstructedStepDefinition);
}

[Fact]
public void ProperlyDeserializesCollectionProperties() {

// The following is from the CCK hooks sample
var json = @"{""id"":""33"",""pickleId"":""20"",""testSteps"":[{""hookId"":""0"",""id"":""29""},{""hookId"":""1"",""id"":""30""},{""id"":""31"",""pickleStepId"":""19"",""stepDefinitionIds"":[""2""],""stepMatchArgumentsLists"":[{""stepMatchArguments"":[]}]},{""hookId"":""4"",""id"":""32""}]}";

// This test will pass if the deserializer does not throw an exception
var testCase = NdjsonSerializer.Deserialize<TestCase>(json);
Assert.Equal(4, testCase.TestSteps.Count);
}
}
}
2 changes: 1 addition & 1 deletion dotnet/Cucumber.Messages/generated/Background.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ string id
RequireNonNull<string>(description, "Description", "Background.Description cannot be null");
this.Description = description;
RequireNonNull<List<Step>>(steps, "Steps", "Background.Steps cannot be null");
this.Steps = new List<Step>(steps);
this.Steps = steps == null ? new List<Step>() : new List<Step>(steps);
Copy link
Contributor

@mpkorstanje mpkorstanje Sep 9, 2024

Choose a reason for hiding this comment

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

How can steps be null? It is guarded by RequireNonNull on the line above.

Copy link
Contributor Author

@clrudolphi clrudolphi Sep 9, 2024

Choose a reason for hiding this comment

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

Hmm. Hadn't been dealing with Background yet, so hadn't seen that. At worst, it's a bit redundant.

But look at TestStep (before my proposed change):

    public TestStep(
        string hookId,
        string id,
        string pickleStepId,
        List<string> stepDefinitionIds,
        List<StepMatchArgumentsList> stepMatchArgumentsLists
    ) 
    {
              this.HookId = hookId;
              RequireNonNull<string>(id, "Id", "TestStep.Id cannot be null");
        this.Id = id;
              this.PickleStepId = pickleStepId;
              this.StepDefinitionIds = new List<string>(stepDefinitionIds);
              this.StepMatchArgumentsLists = new List<StepMatchArgumentsList>(stepMatchArgumentsLists);
    }

The schema here indicates that the list of StepDefinitionIds and the list of StepMatchArgumentsLists are optional.
If this code gets invoked with null for either, the code throws an exception as the List constructor, when given an argument, the argument must not be null.

I added a test in BasicMessageSerializationTests.cs to demonstrate the behavior.

Would you prefer that I make the template a bit more sophisticated to use the new pattern only when the property is optional?

Copy link
Contributor

@mpkorstanje mpkorstanje Sep 9, 2024

Choose a reason for hiding this comment

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

Ah. Yes. More sophisticated would be preferred. You can probably copy this from Java.

RequireNonNull<string>(id, "Id", "Background.Id cannot be null");
this.Id = id;
}
Expand Down
2 changes: 1 addition & 1 deletion dotnet/Cucumber.Messages/generated/DataTable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ List<TableRow> rows
RequireNonNull<Location>(location, "Location", "DataTable.Location cannot be null");
this.Location = location;
RequireNonNull<List<TableRow>>(rows, "Rows", "DataTable.Rows cannot be null");
this.Rows = new List<TableRow>(rows);
this.Rows = rows == null ? new List<TableRow>() : new List<TableRow>(rows);
}

public override bool Equals(Object o)
Expand Down
4 changes: 2 additions & 2 deletions dotnet/Cucumber.Messages/generated/Examples.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ string id
RequireNonNull<Location>(location, "Location", "Examples.Location cannot be null");
this.Location = location;
RequireNonNull<List<Tag>>(tags, "Tags", "Examples.Tags cannot be null");
this.Tags = new List<Tag>(tags);
this.Tags = tags == null ? new List<Tag>() : new List<Tag>(tags);
RequireNonNull<string>(keyword, "Keyword", "Examples.Keyword cannot be null");
this.Keyword = keyword;
RequireNonNull<string>(name, "Name", "Examples.Name cannot be null");
Expand All @@ -52,7 +52,7 @@ string id
this.Description = description;
this.TableHeader = tableHeader;
RequireNonNull<List<TableRow>>(tableBody, "TableBody", "Examples.TableBody cannot be null");
this.TableBody = new List<TableRow>(tableBody);
this.TableBody = tableBody == null ? new List<TableRow>() : new List<TableRow>(tableBody);
RequireNonNull<string>(id, "Id", "Examples.Id cannot be null");
this.Id = id;
}
Expand Down
4 changes: 2 additions & 2 deletions dotnet/Cucumber.Messages/generated/Feature.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ List<FeatureChild> children
RequireNonNull<Location>(location, "Location", "Feature.Location cannot be null");
this.Location = location;
RequireNonNull<List<Tag>>(tags, "Tags", "Feature.Tags cannot be null");
this.Tags = new List<Tag>(tags);
this.Tags = tags == null ? new List<Tag>() : new List<Tag>(tags);
RequireNonNull<string>(language, "Language", "Feature.Language cannot be null");
this.Language = language;
RequireNonNull<string>(keyword, "Keyword", "Feature.Keyword cannot be null");
Expand All @@ -69,7 +69,7 @@ List<FeatureChild> children
RequireNonNull<string>(description, "Description", "Feature.Description cannot be null");
this.Description = description;
RequireNonNull<List<FeatureChild>>(children, "Children", "Feature.Children cannot be null");
this.Children = new List<FeatureChild>(children);
this.Children = children == null ? new List<FeatureChild>() : new List<FeatureChild>(children);
}

public override bool Equals(Object o)
Expand Down
2 changes: 1 addition & 1 deletion dotnet/Cucumber.Messages/generated/GherkinDocument.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ List<Comment> comments
this.Uri = uri;
this.Feature = feature;
RequireNonNull<List<Comment>>(comments, "Comments", "GherkinDocument.Comments cannot be null");
this.Comments = new List<Comment>(comments);
this.Comments = comments == null ? new List<Comment>() : new List<Comment>(comments);
}

public override bool Equals(Object o)
Expand Down
2 changes: 1 addition & 1 deletion dotnet/Cucumber.Messages/generated/Group.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ string value
)
{
RequireNonNull<List<Group>>(children, "Children", "Group.Children cannot be null");
this.Children = new List<Group>(children);
this.Children = children == null ? new List<Group>() : new List<Group>(children);
this.Start = start;
this.Value = value;
}
Expand Down
2 changes: 1 addition & 1 deletion dotnet/Cucumber.Messages/generated/JavaMethod.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ List<string> methodParameterTypes
RequireNonNull<string>(methodName, "MethodName", "JavaMethod.MethodName cannot be null");
this.MethodName = methodName;
RequireNonNull<List<string>>(methodParameterTypes, "MethodParameterTypes", "JavaMethod.MethodParameterTypes cannot be null");
this.MethodParameterTypes = new List<string>(methodParameterTypes);
this.MethodParameterTypes = methodParameterTypes == null ? new List<string>() : new List<string>(methodParameterTypes);
}

public override bool Equals(Object o)
Expand Down
2 changes: 1 addition & 1 deletion dotnet/Cucumber.Messages/generated/ParameterType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ SourceReference sourceReference
RequireNonNull<string>(name, "Name", "ParameterType.Name cannot be null");
this.Name = name;
RequireNonNull<List<string>>(regularExpressions, "RegularExpressions", "ParameterType.RegularExpressions cannot be null");
this.RegularExpressions = new List<string>(regularExpressions);
this.RegularExpressions = regularExpressions == null ? new List<string>() : new List<string>(regularExpressions);
RequireNonNull<bool>(preferForRegularExpressionMatch, "PreferForRegularExpressionMatch", "ParameterType.PreferForRegularExpressionMatch cannot be null");
this.PreferForRegularExpressionMatch = preferForRegularExpressionMatch;
RequireNonNull<bool>(useForSnippets, "UseForSnippets", "ParameterType.UseForSnippets cannot be null");
Expand Down
6 changes: 3 additions & 3 deletions dotnet/Cucumber.Messages/generated/Pickle.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,11 @@ List<string> astNodeIds
RequireNonNull<string>(language, "Language", "Pickle.Language cannot be null");
this.Language = language;
RequireNonNull<List<PickleStep>>(steps, "Steps", "Pickle.Steps cannot be null");
this.Steps = new List<PickleStep>(steps);
this.Steps = steps == null ? new List<PickleStep>() : new List<PickleStep>(steps);
RequireNonNull<List<PickleTag>>(tags, "Tags", "Pickle.Tags cannot be null");
this.Tags = new List<PickleTag>(tags);
this.Tags = tags == null ? new List<PickleTag>() : new List<PickleTag>(tags);
RequireNonNull<List<string>>(astNodeIds, "AstNodeIds", "Pickle.AstNodeIds cannot be null");
this.AstNodeIds = new List<string>(astNodeIds);
this.AstNodeIds = astNodeIds == null ? new List<string>() : new List<string>(astNodeIds);
}

public override bool Equals(Object o)
Expand Down
2 changes: 1 addition & 1 deletion dotnet/Cucumber.Messages/generated/PickleStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ string text
{
this.Argument = argument;
RequireNonNull<List<string>>(astNodeIds, "AstNodeIds", "PickleStep.AstNodeIds cannot be null");
this.AstNodeIds = new List<string>(astNodeIds);
this.AstNodeIds = astNodeIds == null ? new List<string>() : new List<string>(astNodeIds);
RequireNonNull<string>(id, "Id", "PickleStep.Id cannot be null");
this.Id = id;
this.Type = type;
Expand Down
2 changes: 1 addition & 1 deletion dotnet/Cucumber.Messages/generated/PickleTable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ List<PickleTableRow> rows
)
{
RequireNonNull<List<PickleTableRow>>(rows, "Rows", "PickleTable.Rows cannot be null");
this.Rows = new List<PickleTableRow>(rows);
this.Rows = rows == null ? new List<PickleTableRow>() : new List<PickleTableRow>(rows);
}

public override bool Equals(Object o)
Expand Down
2 changes: 1 addition & 1 deletion dotnet/Cucumber.Messages/generated/PickleTableRow.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ List<PickleTableCell> cells
)
{
RequireNonNull<List<PickleTableCell>>(cells, "Cells", "PickleTableRow.Cells cannot be null");
this.Cells = new List<PickleTableCell>(cells);
this.Cells = cells == null ? new List<PickleTableCell>() : new List<PickleTableCell>(cells);
}

public override bool Equals(Object o)
Expand Down
4 changes: 2 additions & 2 deletions dotnet/Cucumber.Messages/generated/Rule.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,15 @@ string id
RequireNonNull<Location>(location, "Location", "Rule.Location cannot be null");
this.Location = location;
RequireNonNull<List<Tag>>(tags, "Tags", "Rule.Tags cannot be null");
this.Tags = new List<Tag>(tags);
this.Tags = tags == null ? new List<Tag>() : new List<Tag>(tags);
RequireNonNull<string>(keyword, "Keyword", "Rule.Keyword cannot be null");
this.Keyword = keyword;
RequireNonNull<string>(name, "Name", "Rule.Name cannot be null");
this.Name = name;
RequireNonNull<string>(description, "Description", "Rule.Description cannot be null");
this.Description = description;
RequireNonNull<List<RuleChild>>(children, "Children", "Rule.Children cannot be null");
this.Children = new List<RuleChild>(children);
this.Children = children == null ? new List<RuleChild>() : new List<RuleChild>(children);
RequireNonNull<string>(id, "Id", "Rule.Id cannot be null");
this.Id = id;
}
Expand Down
6 changes: 3 additions & 3 deletions dotnet/Cucumber.Messages/generated/Scenario.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,17 @@ string id
RequireNonNull<Location>(location, "Location", "Scenario.Location cannot be null");
this.Location = location;
RequireNonNull<List<Tag>>(tags, "Tags", "Scenario.Tags cannot be null");
this.Tags = new List<Tag>(tags);
this.Tags = tags == null ? new List<Tag>() : new List<Tag>(tags);
RequireNonNull<string>(keyword, "Keyword", "Scenario.Keyword cannot be null");
this.Keyword = keyword;
RequireNonNull<string>(name, "Name", "Scenario.Name cannot be null");
this.Name = name;
RequireNonNull<string>(description, "Description", "Scenario.Description cannot be null");
this.Description = description;
RequireNonNull<List<Step>>(steps, "Steps", "Scenario.Steps cannot be null");
this.Steps = new List<Step>(steps);
this.Steps = steps == null ? new List<Step>() : new List<Step>(steps);
RequireNonNull<List<Examples>>(examples, "Examples", "Scenario.Examples cannot be null");
this.Examples = new List<Examples>(examples);
this.Examples = examples == null ? new List<Examples>() : new List<Examples>(examples);
RequireNonNull<string>(id, "Id", "Scenario.Id cannot be null");
this.Id = id;
}
Expand Down
2 changes: 1 addition & 1 deletion dotnet/Cucumber.Messages/generated/StepMatchArgumentsList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ List<StepMatchArgument> stepMatchArguments
)
{
RequireNonNull<List<StepMatchArgument>>(stepMatchArguments, "StepMatchArguments", "StepMatchArgumentsList.StepMatchArguments cannot be null");
this.StepMatchArguments = new List<StepMatchArgument>(stepMatchArguments);
this.StepMatchArguments = stepMatchArguments == null ? new List<StepMatchArgument>() : new List<StepMatchArgument>(stepMatchArguments);
}

public override bool Equals(Object o)
Expand Down
2 changes: 1 addition & 1 deletion dotnet/Cucumber.Messages/generated/TableRow.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ string id
RequireNonNull<Location>(location, "Location", "TableRow.Location cannot be null");
this.Location = location;
RequireNonNull<List<TableCell>>(cells, "Cells", "TableRow.Cells cannot be null");
this.Cells = new List<TableCell>(cells);
this.Cells = cells == null ? new List<TableCell>() : new List<TableCell>(cells);
RequireNonNull<string>(id, "Id", "TableRow.Id cannot be null");
this.Id = id;
}
Expand Down
2 changes: 1 addition & 1 deletion dotnet/Cucumber.Messages/generated/TestCase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ List<TestStep> testSteps
RequireNonNull<string>(pickleId, "PickleId", "TestCase.PickleId cannot be null");
this.PickleId = pickleId;
RequireNonNull<List<TestStep>>(testSteps, "TestSteps", "TestCase.TestSteps cannot be null");
this.TestSteps = new List<TestStep>(testSteps);
this.TestSteps = testSteps == null ? new List<TestStep>() : new List<TestStep>(testSteps);
}

public override bool Equals(Object o)
Expand Down
4 changes: 2 additions & 2 deletions dotnet/Cucumber.Messages/generated/TestStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ List<StepMatchArgumentsList> stepMatchArgumentsLists
RequireNonNull<string>(id, "Id", "TestStep.Id cannot be null");
this.Id = id;
this.PickleStepId = pickleStepId;
this.StepDefinitionIds = new List<string>(stepDefinitionIds);
this.StepMatchArgumentsLists = new List<StepMatchArgumentsList>(stepMatchArgumentsLists);
this.StepDefinitionIds = stepDefinitionIds == null ? new List<string>() : new List<string>(stepDefinitionIds);
Copy link
Contributor

@mpkorstanje mpkorstanje Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this should be:

Suggested change
this.StepDefinitionIds = stepDefinitionIds == null ? new List<string>() : new List<string>(stepDefinitionIds);
this.StepDefinitionIds = stepDefinitionIds == null ? null : new List<string>(stepDefinitionIds);

Similar to https://github.com/cucumber/messages/blob/de3f88f1b7fa43ebceeb6e247767e44ff8c18730/java/src/generated/java/io/cucumber/messages/types/TestStep.java#L36C1-L36C122

The field is not required, so it can be null. When serializing, if configured correctly, this will result in the omission of the field. And so it would be strange if non-required value that wasn't present suddenly became an empty list after serialisation.

Copy link
Contributor Author

@clrudolphi clrudolphi Sep 10, 2024

Choose a reason for hiding this comment

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

In principle, I agree with you, purely optional elements should be null and not emitted when serialized.
I'm curious then about these lines in the Hooks example in the CCK:

{"testCase":{"id":"33","pickleId":"20","testSteps":[{"hookId":"0","id":"29"},{"hookId":"1","id":"30"},{"id":"31","pickleStepId":"19","stepDefinitionIds":["2"],"stepMatchArgumentsLists":[{"stepMatchArguments":[]}]},{"hookId":"4","id":"32"}]}}
{"testCase":{"id":"38","pickleId":"22","testSteps":[{"hookId":"0","id":"34"},{"hookId":"1","id":"35"},{"id":"36","pickleStepId":"21","stepDefinitionIds":["3"],"stepMatchArgumentsLists":[{"stepMatchArguments":[]}]},{"hookId":"4","id":"37"}]}}
{"testCase":{"id":"43","pickleId":"24","testSteps":[{"hookId":"0","id":"39"},{"hookId":"1","id":"40"},{"id":"41","pickleStepId":"23","stepDefinitionIds":[],"stepMatchArgumentsLists":[]},{"hookId":"4","id":"42"}]}}

In the first two lines, the third step of each test case is matched with a StepDefinition; each of those stepDefinitions accept no parameters. So why are the stepMatchArgumentsLists and stepMatchArguments properties being serialized as empty lists? Why not omit them altogether? (They are optional according to the schema).
Likewise in the third example (the test case does not bind to any step definition (aka, pending). Why is the StepDefinitionIds array getting serialized when it is empty?

I will try out your proposal and see how well it works with Reqnroll.

Copy link
Contributor

@mpkorstanje mpkorstanje Sep 11, 2024

Choose a reason for hiding this comment

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

A TestStep is either a PickleTestStep or a HookTestStep.

  • When a PickleTestStep then the id, pickleStepId, stepDefinitionIds and stepMatchArgumentsLists are required.
  • When a HookTestStep then the id and hookId are required.

So in the union of these, only the id is required.

There are now better ways to express this in a json schemas but either we didn't have those at the time, or they didn't quite work.

Copy link
Contributor Author

@clrudolphi clrudolphi Sep 11, 2024

Choose a reason for hiding this comment

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

Oof. That makes a general solution almost impossible.
If the template were to deserialize missing optional lists as null then they won't round-trip correctly. (Nor would it round-trip correctly with the code as it exists or with what I had proposed)
I'll consider inserting a patch up phase in the Serializer helper class.

Copy link
Contributor

@mpkorstanje mpkorstanje Sep 11, 2024

Choose a reason for hiding this comment

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

How does it make round tripping correctly impossible? Not including null values when serializing and not replacing absent/null values with empty list during deserialisation is a successful round trip.

Or do you mean with the current implementation?

Copy link
Contributor Author

@clrudolphi clrudolphi Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Well, OK, not impossible.
Under good conditions, the round-trip would be successful.
Under the principle of being forgiving of bad input and strict on output; if the input json string (for a Pickle TestStep) were to omit the stepDefinitionIds instead of representing it as an empty array, then this code would set the StepDefinitionIds property to null, and render it back out during serialization as null (rather than the required empty array).
Are those edge cases something that we should be guarding against?

Copy link
Contributor

@mpkorstanje mpkorstanje Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There might be a use-case for it, but it would be somewhat out of scope for this PR.

In this example, an error correction to an empty list would be context sensitive. The correct solution would depend on whether we are dealing with a PickleTestStep or a HookTestStep.

Given that sort of complexity, probably best discussed separately, if at all.

this.StepMatchArgumentsLists = stepMatchArgumentsLists == null ? new List<StepMatchArgumentsList>() : new List<StepMatchArgumentsList>(stepMatchArgumentsLists);
}

public override bool Equals(Object o)
Expand Down