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.

Sign up for GitHub

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

Jump to bottom

[cpp-pistache] add the option to fetch dependencies #495

Merged
merged 5 commits into from
Jul 22, 2018
Merged

[cpp-pistache] add the option to fetch dependencies #495

merged 5 commits into from
Jul 22, 2018

Conversation

etherealjoy
Copy link
Contributor

@etherealjoy etherealjoy commented Jul 7, 2018 •
edited
Loading

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.1.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 External Libraries as downloaded projects to compile the code
Add generation of petstore without recompiling the jar

@stkrwork @MartinDelille

java -jar ./openapi-generator-cli.jar config-help -g cpp-pistache-server

produces

CONFIG OPTIONS
	addExternalLibs
	    Add the Possibility to fetch and compile external Libraries needed by this Framework.
(Default: false)

This was referenced Jul 7, 2018
Closed
Merged
etherealjoy
etherealjoy commented Jul 8, 2018
View reviewed changes
...api-generator/src/main/java/org/openapitools/codegen/languages/CppPistacheServerCodegen.java Outdated

protected boolean isAddExternalLibs = true;
public static final String OPTIONAL_EXTERNAL_LIB = "addExternalLibs";
public static final String OPTIONL_EXTERNAL_LIB_DESC = "Add the Possibility to fetch and compile external Libraries needed by this Framework.";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

typo 👎

etherealjoy
etherealjoy commented Jul 14, 2018
View reviewed changes
...api-generator/src/main/java/org/openapitools/codegen/languages/CppPistacheServerCodegen.java Outdated
@@ -43,7 +43,9 @@

public class CppPistacheServerCodegen extends AbstractCppCodegen {
protected String implFolder = "impl";

protected boolean isAddExternalLibs = true;
Copy link
Contributor Author

@etherealjoy etherealjoy Jul 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@stkrwork
Should we make this default false?
Normally when it is true we can build it out of the box.

Copy link
Member

Choose a reason for hiding this comment

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

@etherealjoy I suggest either merging it to 3.2.x (breaking changes without fallback) if defaults to true or change the default to false if we want to merge this PR into current master.

@wing328 wing328 modified the milestone: 3.1.2 Jul 20, 2018
@etherealjoy
Copy link
Contributor Author

@wing328
I updated to make addExternalLibs default false

--additional-properties addExternalLibs=true

Just add this to the command line and you can build everything on the fly without any changes to your environment.
This will pull the deps in separate folder and build them and add include and link locations accordingly.

@wing328
Copy link
Member

wing328 commented Jul 21, 2018

@etherealjoy thanks. Can you please resolve the merge conflicts? Then I'll merge it into master.

@etherealjoy
Rebase and resolve merge conflicts
ce7b437 
Merge remote-tracking branch 'remotes/upstream/master' into cpp-pistache-add-deps

# Conflicts:
#	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/CppPistacheServerCodegen.java
@etherealjoy
Copy link
Contributor Author

@wing328
Resolved the merge conflicts.

wing328
wing328 reviewed Jul 21, 2018
View reviewed changes
...api-generator/src/main/java/org/openapitools/codegen/languages/CppPistacheServerCodegen.java Outdated
@Override
public String getTypeDeclaration(String str) {
return toModelName(str);
}

/**
* specify whether external libraries will be added during the generation
Copy link
Member

Choose a reason for hiding this comment

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

@etherealjoy I think you will need to add a proper docstring for the parameter "value" to address the following error reported by the CI:


[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.0.1:jar (attach-javadocs) on project openapi-generator: MavenReportException: Error while generating Javadoc: Project contains Javadoc Warnings -> [Help 1]
[ERROR]

@etherealjoy etherealjoy mentioned this pull request Jul 21, 2018
Merged
4 tasks
@wing328 wing328 added this to the 3.1.2 milestone Jul 22, 2018
@wing328 wing328 merged commit 1dee3e2 into OpenAPITools:master Jul 22, 2018
@wing328
Copy link
Member

wing328 commented Jul 22, 2018

@etherealjoy thanks. The PR has been merged into master.

@etherealjoy etherealjoy deleted the cpp-pistache-add-deps branch July 22, 2018 03:58
@CyrilleBenard
Copy link

@etherealjoy You wrote above that the addExternalLibs default is false but when have a look to the generator config-help for cpp-pistache-server I see :

CONFIG OPTIONS
	addExternalLibs
	    Add the Possibility to fetch and compile external Libraries needed by this Framework. (Default: true)

Is it only a documentation issue ?

@wing328
Copy link
Member

wing328 commented Jul 31, 2018

It was false in 3.1.x. In 3.2.0 (current master), we've enabled it as the default (and mark that as a breaking change)

@CyrilleBenard
Copy link

Ah ok, I missed that.
Nice 👍

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
@etherealjoy @wing328
[cpp-pistache] add the option to fetch dependencies (OpenAPITools#495)
31de6ad 
* Add the Possibility to fetch dependencies needed by the generated code

* Fix typo

* Make External Libraries default to false

* Add parameter string to the javadoc comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wing328 wing328 wing328 left review comments

Assignees
No one assigned
Labels
Enhancement: Feature Server: C++
Projects
None yet
Milestone
3.1.2
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@etherealjoy @wing328 @CyrilleBenard