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

Add support for multiple inheritance #1664

Merged
merged 15 commits into from Dec 14, 2018

Conversation

Projects
None yet
2 participants
@wing328
Copy link
Member

wing328 commented Dec 11, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell 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). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

  • add support for multiple inheritances: allParents tag
  • bug fixes for allVars, vars
  • fix allRequired
  • fix property naming in mandatory, allMandatory
  • add more test cases
  • add isSelfReference flag to codegen property
  • update run-in-docker.sh to use the local Maven cache

cc @OpenAPITools/generator-core-team

@auto-labeler auto-labeler bot added the bug label Dec 11, 2018

@auto-labeler

This comment has been minimized.

Copy link

auto-labeler bot commented Dec 11, 2018

Thanks for opening this issue! I have applied any relevant labels.

@wing328 wing328 added Issue: Bug and removed bug labels Dec 12, 2018

@cbornet

This comment has been minimized.

Copy link
Member

cbornet commented Dec 12, 2018

Didn't we already have supportsMixin for language that support "multiple inheritance" ?

@wing328 wing328 changed the title [WIP] Add support for multiple inheritance Add support for multiple inheritance Dec 12, 2018

@wing328

This comment has been minimized.

Copy link
Member

wing328 commented Dec 12, 2018

@cbornet yup, only JS client generator uses that.

My current approach is to keep the "supportsMixin" and add new code, mustache tags to support mutiple inheritnance for other langauges as part of the code cleanup/refactoring.

@wing328 wing328 removed the WIP label Dec 12, 2018

@cbornet

This comment has been minimized.

Copy link
Member

cbornet commented Dec 12, 2018

I mean, can't we reuse this option for other languages ?

@wing328

This comment has been minimized.

Copy link
Member

wing328 commented Dec 13, 2018

Very good question. Ideally we should extend or reuse supportsMixin for other languages (in addition to JS) but when I read the following code:

if (modelImplCnt++ > 1) {
LOGGER.warn("More than one inline schema specified in allOf:. Only the first one is recognized. All others are ignored.");
break; // only one schema with discriminator allowed in allOf
}

I couldn't understand why only the first model is considered in mixin (multiple inheritance) - shoudn't it consider all models defined in allOf as it's multiple inheritance?. I don't want to modify it least it break the JS API client. That's why I introduce a new option instead and add the allParents mustache tag should a language need multiple inheritance/mixins. We can later migrate JS to use supportsMultipleInheritance to migrate risks in breaking existing usage.

@cbornet

This comment has been minimized.

Copy link
Member

cbornet commented Dec 13, 2018

Actually with your modifications supportsMixins doesn't have any effect anymore so it could just be removed...

@cbornet

This comment has been minimized.

Copy link
Member

cbornet commented Dec 13, 2018

Also I prefer "mixin" over "multiple inheritance" as it better describe what is done. Multiple inheritance is bad and should be avoided at all cost while mixins are ok.

@wing328

This comment has been minimized.

Copy link
Member

wing328 commented Dec 13, 2018

Also I prefer "mixin" over "multiple inheritance" as it better describe what is done. Multiple inheritance is bad and should be avoided at all cost while mixins are ok.

OK. I'll keep the supportsMixins name.

@cbornet

This comment has been minimized.

Copy link
Member

cbornet commented Dec 13, 2018

👍 I wonder what's the effect of the modifications on the JS codegen ? I guess the mixin/multiple-inheritance shall be activated for this one.

@wing328

This comment has been minimized.

Copy link
Member

wing328 commented Dec 14, 2018

I wonder what's the effect of the modifications on the JS codegen?

I've updated the JS samples and the tests passed. No visible code change which is expected as I try not to break backward compatibility at the moment.

@wing328 wing328 merged commit 8c599eb into master Dec 14, 2018

6 checks passed

Shippable Run 4647 status is SUCCESS.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@cbornet

This comment has been minimized.

Copy link
Member

cbornet commented Dec 14, 2018

Also I don't think we have multiple inheritance in samples

@wing328

This comment has been minimized.

Copy link
Member

wing328 commented Dec 17, 2018

@cbornet yup, I added several spec to test allOf, multiple inheritance, etc (e.g. allOfMultiParent.yaml). We'll add those to the fake petstore spec when more generators are ready for these new features. We'll test with these new spec individually for the time being.

@wing328 wing328 deleted the multiple_parent branch Dec 17, 2018

@wing328 wing328 referenced this pull request Dec 21, 2018

Closed

[WIP] Add logic to detect self-reference models #1548

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment