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.

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

[csharp] Support inheritance instead of duplicating parent properties in derived classes #5922

Merged
merged 30 commits into from
Sep 6, 2017
Merged

[csharp] Support inheritance instead of duplicating parent properties in derived classes #5922

merged 30 commits into from
Sep 6, 2017

Conversation

manuc66
Copy link
Contributor

@manuc66 manuc66 commented Jun 24, 2017

PR checklist

  • Read the contribution guidelines.
  • Ran the shell/batch script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates)
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

Took the work done in PR #4080: "[csharp] Don't duplicate parent properties in derived classes".

Fixed the generated code for:

  • validation with the problem of calling explicit implementation on the base class.
  • overriding method ToJson() instead of hiding
  • re-use the parent GetHashCode
  • defining the default value of discriminator property in child type to the typeName

Fixed some ".bat":

  • csharp-dotnet2-petstore.bat was missing
  • csharp-property-changed-petstore.bat did not contains the right arguments vs the .sh version

Included an leveraged a helper library in order to de-serialize a child class with Newtonsoft.Json, the original library is hosted here: https://github.com/manuc66/JsonSubTypes (this feature is not available in Newtonsoft.Json, see: JamesNK/Newtonsoft.Json#1331

jimschubert and others added 21 commits October 17, 2016 16:21
This includes supportsInheritance only for the client codegen at the
moment, because setting in AbstractCSharpCodegen would require the
change to be tested in all derived generators, possibly including
similar template changes to this commit's.
…into feature/csharp-subtypes2

# Conflicts:
#	modules/swagger-codegen/src/main/resources/csharp/modelGeneric.mustache
…codegen into feature/csharp-subtypes2

# Conflicts:
#	modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/CSharpClientCodegen.java
#	modules/swagger-codegen/src/main/resources/csharp/modelGeneric.mustache
- bin/csharp-petstore-all.sh && bin/security/csharp-petstore.sh
- bin/csharp-dotnet2-petstore.sh && bin/csharp-petstore.sh && bin/csharp-petstore-netcore-project.sh && bin/csharp-petstore-net-standard.sh && bin/csharp-property-changed-petstore.sh
+ fix the csharp-property-changed-petstore.bat
@manuc66 manuc66 changed the title Feature/csharp subtypes [csharp] Support of inheritance instead of duplicating parent properties in derived classes Jun 24, 2017
@manuc66 manuc66 changed the title [csharp] Support of inheritance instead of duplicating parent properties in derived classes [csharp] Support inheritance instead of duplicating parent properties in derived classes Jun 24, 2017
@manuc66
Copy link
Contributor Author

manuc66 commented Jun 29, 2017

Seems the Travis error is not related to this changeset, could the build be re-triggered ?


namespace JsonSubTypes
{
// Copyright 2017 Emmanuel Counasse
Copy link
Contributor

Choose a reason for hiding this comment

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

@manuc66 Can we remove the copyright note and license here? The auto-generated code in any languages should be unlicensed by default.

Copy link
Contributor Author

@manuc66 manuc66 Jul 1, 2017

Choose a reason for hiding this comment

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

I chose to include the small library as source code instead of an independently licensed package dependency that would have increase constraints both on the consumer and producer side. I thought it would not be a problem as I saw that copyright notices exist in other generated files.

However, I think the copyright notice could be removed as long a little notice explain that it came from 'https://github.com/manuc66/JsonSubTypes'.

What do you think about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@manuc66 What about putting down something similar to

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, it's done ;-)

@wing328
Copy link
Contributor

wing328 commented Jul 1, 2017

@manuc66 don't worry about the Travis error. I'll restart it and test locally if it's still failing. (C# related tests are covered by AppVeyor)

@wing328
Copy link
Contributor

wing328 commented Jul 1, 2017

cc @jimschubert

@manuc66
Copy link
Contributor Author

manuc66 commented Jul 30, 2017

Conflicts solved

@wing328
Copy link
Contributor

wing328 commented Sep 6, 2017

@manuc66 I performed a few more tests and the result is good. Thanks for the contribution.

@wing328 wing328 merged commit a41e8be into swagger-api:master Sep 6, 2017
@manuc66 manuc66 deleted the feature/csharp-subtypes branch September 6, 2017 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants