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

[cpp-pistache] removed model namespace when unused for operations #775

Merged

Conversation

etherealjoy
Copy link
Contributor

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, 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

When there is no import of any model header remove the usage of model namespace as well for each model.
No impact to samples after generation.
Fixes #637 fully.

@ravinikam @MartinDelille @stkrwork @fvarose

@@ -250,7 +250,7 @@ public CodegenOperation fromOperation(String path, String httpMethod, Operation
if(importMapping.containsKey(hdr)) {
continue;
}
additionalProperties.put("hasModelImport", true);
operations.put("hasModelImport", true);
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldnt it better to put this in the vendor extensions?

Copy link
Member

Choose a reason for hiding this comment

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

We may consider adding this to the top level so that all generators can also use hasModelImport (I've seen another related request before)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wing328 @stkrwork
Then in this case would it better to use vendor extensions or in this similar way as done here?

Copy link
Member

Choose a reason for hiding this comment

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

@etherealjoy please go with vendor extension for the time being (e.g. x-codegen-pistache-hasModelImport)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wing328 @stkrwork
Ok, done using vendor extensions

@etherealjoy
Copy link
Contributor Author

@wing328 @stkrwork
Ok vendor extension is only at operation level but not at API level. therefore we cannot use vendor extension.
I need to put the variable at operations level not at operation level.
The suggestion is not working unless we add vendor extension at operations level.

@etherealjoy
Copy link
Contributor Author

etherealjoy commented Aug 21, 2018

@wing328 @stkrwork
vendorExtensions is a member of CodegenOperation, but operations in method

private Map<String, Object> processOperations(CodegenConfig config, String tag, \
List<CodegenOperation> ops, List<Object> allModels)

is a Map<String, Object> and contains the mustache flag hasImport as well.

if (imports.size() > 0) {
     operations.put("hasImport", true);
}

So I think it makes sense to add hasModelImport here as well in the overriden method. vendorExtensions is only at operation level and is not working.
I reverted the change back to not use vendorExtensions and if you think it is OK we can merge and I have tested it.

@etherealjoy etherealjoy removed this from the 3.2.2 milestone Aug 21, 2018
@wing328
Copy link
Member

wing328 commented Aug 24, 2018

@etherealjoy thanks for the detailed explanation. I'm ok with your suggested change.

We'll consider adding this to the default codegen later.

…ons level instead of operation/vendorExtensions level.
@etherealjoy etherealjoy added this to the 3.2.3 milestone Aug 24, 2018
@etherealjoy etherealjoy merged commit 6a00b2a into OpenAPITools:master Aug 24, 2018
@etherealjoy etherealjoy deleted the pistache_fix_model_namespace branch August 24, 2018 09:34
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
…enAPITools#775)

* Remove using model namespace when model is unused
* Add comments to clarify introduction of hasModelImport at API/operations level instead of operation/vendorExtensions level.
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.

None yet

3 participants