-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
[DotNet] Fix generation of classes that accept optional lists as constructor arguments #249
Changes from 1 commit
bf1407e
fc7edd5
b099347
fc7ee47
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How can There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be:
Suggested change
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
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). I will try out your proposal and see how well it works with Reqnroll. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A
So in the union of these, only the 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oof. That makes a general solution almost impossible. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, OK, not impossible. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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) | ||||||
|
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.
Spotted this after merging. Changes should be listed as one of
Added
,Changed
, ect. I'll fix it on main.