Skip to content

Adds support for Copy Constructors #853

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

Merged
merged 14 commits into from
Jul 12, 2022

Conversation

MaggieKimani1
Copy link
Contributor

Fixes #832

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

wouldn't it make more sense to implement ICloneable semantically instead?
Also I noticed that in every copy constructor the code is just copying the reference for properties, instead of making a copy of the object. That means that a change on myOriginal.PropA.PropAlpha = "foo" would also impact the copy myCopy.PropA.PropAlpha is now also valued at "foo". This could have tons of side effects.
See how I implemented it for Kiota https://github.com/microsoft/kiota/blob/d19d14e06bc6fcadac91d208d2bf1c54c1a8918c/src/Kiota.Builder/CodeDOM/CodeMethod.cs#L160

@MaggieKimani1
Copy link
Contributor Author

wouldn't it make more sense to implement ICloneable semantically instead? Also I noticed that in every copy constructor the code is just copying the reference for properties, instead of making a copy of the object. That means that a change on myOriginal.PropA.PropAlpha = "foo" would also impact the copy myCopy.PropA.PropAlpha is now also valued at "foo". This could have tons of side effects. See how I implemented it for Kiota https://github.com/microsoft/kiota/blob/d19d14e06bc6fcadac91d208d2bf1c54c1a8918c/src/Kiota.Builder/CodeDOM/CodeMethod.cs#L160

Do you mind expounding on the side effects?
Also implementing ICloneable semantically as you've done in Kiota would require type-casting when one is trying to mutate the cloned object?

@matt9ucci
Copy link
Contributor

Seems to be related with #537. I hope ICloneable, no side effects and no hierarchy problems, is implemented for all models.

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

thanks for making updates, still a few changes required

@darrelmiller
Copy link
Member

The API design that was approved for minimal APIs was to be able to use copy constructors. By having copy constructors it allows creating a new instance of the object and then using object initializers to set values. e.g.

app.MapGet("/foo", () => {})
.WithOpenApi(generatedOperation => new(generatedOperation) {
  OperationId = "MadeByMe"
});

It is not possible to do the same with ICloneable. Can we switch this to use copy constructors?

Also, it is not necessary to clone strings. Strings are immutable and so there is no harm in having the clone share a reference to the string. If one of the properties is updated, it will not affect the other.

@baywet
Copy link
Member

baywet commented May 17, 2022

sorry for the confusion, I didn't have the additional context on this one.

@MaggieKimani1
Copy link
Contributor Author

Thank you for the clarification. I'll revert the PR to the state where I had implemented the copy constructors.

@MaggieKimani1 MaggieKimani1 force-pushed the mk/add-copy-constructors-for-models branch from a35ab9c to c61e1cb Compare May 23, 2022 11:11
@MaggieKimani1 MaggieKimani1 merged commit 6a3e948 into vnext Jul 12, 2022
@MaggieKimani1 MaggieKimani1 deleted the mk/add-copy-constructors-for-models branch July 12, 2022 11:09
@RabebOthmani RabebOthmani mentioned this pull request Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for copy constructors to Microsoft.OpenApi.Models objects
4 participants