[C++][Pistache] Fix optional error and wrong function signatures #264

stkrwork · 2018-06-09T15:19:05Z

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: Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

(details of the change, additional tests that have been done, reference to the issue for tracking, etc)

#257 #258

stkrwork · 2018-06-10T10:33:36Z

@ravinikam @fvarose @etherealjoy @MartinDelille

etherealjoy · 2018-06-10T12:47:14Z

...api-generator/src/main/java/org/openapitools/codegen/languages/CppPistacheServerCodegen.java

@@ -327,29 +327,16 @@ public String toDefaultValue(Schema p) {
            ArraySchema ap = (ArraySchema) p;
            String inner = getSchemaType(ap.getItems());
            if (!languageSpecificPrimitives.contains(inner)) {
-                inner = "std::shared_ptr<" + inner + ">";
+                inner = inner;


Why this assignment?

oh, seems i forgot to remove it, when removing shared ptr

etherealjoy · 2018-06-10T12:50:22Z

samples/server/petstore/pistache-server/model/Pet.cpp

 {
    return m_Category;
 }
-void Pet::setCategory(std::shared_ptr<Category> value)
+void Pet::setCategory(Category value)


Here not a reference?

i'll add it

etherealjoy · 2018-06-10T12:50:43Z

samples/server/petstore/pistache-server/model/Pet.h

-    std::shared_ptr<Category> getCategory() const;
-    void setCategory(std::shared_ptr<Category> value);
+    Category getCategory() const;
+    void setCategory(Category value);


Here not a reference?

i'll add it

etherealjoy · 2018-06-10T13:28:06Z

@stkrwork
Passing complex types by reference makes sense since it avoid copy construction. But Primitive types benefit from passing by value instead of by reference.
Does it makes sense to do this also here. I mean for all purposes the functional behavior is the same.
But the PR is solving some long standing issue in the generator with Optional, which is much needed.

MartinDelille · 2018-06-10T13:43:54Z

I don’t know especially what Pistache is but passing the value by const reference, even for complex type, should be generalized to all cpp generators. What do you think?

stkrwork · 2018-06-10T14:03:37Z

i was considering const&, but in my initial thought i was thinking of doing it for every signature, but this approach is better.

MartinDelille · 2018-06-10T14:05:59Z

Each time I consider using const attributes for my methods, it leads to big and healthy refactoring! 😄

stkrwork · 2018-06-10T14:18:51Z

the const and reference refactoring will most likely also be required for the restbed generator

MartinDelille · 2018-06-10T14:20:46Z

And for Qt generator too! I create the issue.

etherealjoy · 2018-06-10T14:53:28Z

@stkrwork
I have a question, now the complex types have to be const before passing as reference to the api right?

stkrwork · 2018-06-10T14:57:33Z

It just states that the complex type wont be modified. they dont have to be const outside of the function, they are just dealt as constants in the function

stkrwork · 2018-06-10T15:05:54Z

@CyrilleBenard does that fix your issues #257 and #258

CyrilleBenard · 2018-06-10T17:34:25Z

@stkrwork I'm sorry, I still do not try your PR 'cause I evaluate the openapi-generator during my time job. I will do it during the next week. Thx for your reactivity

CyrilleBenard · 2018-06-12T11:59:59Z

One regression seems to occurred, see issue #257 - part 2 of my last comment relative to the use of static + const

stkrwork · 2018-06-12T12:27:11Z

i added your request

wing328 · 2018-06-14T11:39:23Z

If no further question/feedback on this PR, I'll merge it into master in a few hours.

cc @ravinikam @fvarose @etherealjoy @MartinDelille

CyrilleBenard · 2018-06-14T11:50:35Z

Optional error has been fixed but I as far as I know, the function signature is still buggy. May be I misunderstand what's behind "wrong function signatures" (?).
In my opinion, it refers to #258 and nothing has been fixed :/

EDIT: More over, the Optional error fix implies a new issue (see type conversion). I saw in the chat (gitter) that @stkrwork was working on it

stkrwork · 2018-06-14T14:16:20Z

well, it might take some time, because i don't have much time at the moment to implement the ideas i posted in chat

CyrilleBenard · 2018-06-14T14:32:55Z

@stkrwork yes sure 👍
I just replied to @wing328's question :)
Hopping the C++ server stub will compile again as soon as possible, maybe next week ? 💃

stkrwork · 2018-06-14T14:54:16Z

otherwise you can also pick up where i left off.

wing328 · 2018-06-14T16:41:23Z

I'll put this on hold and will only merge this after I get the sign-off from you guys. Please take your time.

Schrigga · 2018-06-17T15:59:48Z

@stkrwork I forked your branch with the changes you have already done. I changed some things, but I stucked compiling the UserApi.cpp when the users should be initialized using an array or an list.
Currently I'm wondering if there is a possibility in the mustache files to distinguish whether there are non primitive type and are arrays. Do you have any knowledge about this?

wing328 · 2018-06-17T16:09:33Z

@Schrigga what about using the following?

{{#isListContainer}}
{{^isPrimitiveType}}
...
{{/isPrimitiveType}}
{{/isListContainer}}

…s in setters that are not primitives

stkrwork mentioned this pull request Jun 9, 2018

[pistache-server](C++ stub) - POST not handled correctly - compile failed #258

Closed

stkrwork changed the title ~~[C++][Pistache] Looking into optional error and wrong function signatures~~ [C++][Pistache] Fix optional error and wrong function signatures Jun 9, 2018

stkrwork force-pushed the pistahce_optional_and_api_fix branch from 7258341 to d77fc9d Compare June 9, 2018 20:07

etherealjoy requested changes Jun 10, 2018

View reviewed changes

MartinDelille mentioned this pull request Jun 10, 2018

[cpp] pass arguments as const reference #272

Closed

MartinDelille approved these changes Jun 10, 2018

View reviewed changes

stkrwork force-pushed the pistahce_optional_and_api_fix branch from 7b90d3a to 9f76d1a Compare June 10, 2018 14:54

etherealjoy approved these changes Jun 10, 2018

View reviewed changes

jimschubert added the Server: C++ label Jun 10, 2018

CyrilleBenard mentioned this pull request Jun 12, 2018

[pistache-server] (C++ stub) - Optional type unknown - compile failed #257

Closed

stkrwork force-pushed the pistahce_optional_and_api_fix branch from 41d8319 to e0b1917 Compare June 12, 2018 12:25

wing328 added the Issue: Bug label Jun 12, 2018

wing328 added this to the 3.0.2 milestone Jun 14, 2018

stkrwork force-pushed the pistahce_optional_and_api_fix branch from cf3db65 to d018fe0 Compare June 16, 2018 09:46

stkrwork force-pushed the pistahce_optional_and_api_fix branch from d018fe0 to c6c1da4 Compare June 17, 2018 18:43

jmini modified the milestones: 3.0.2, 3.0.3 Jun 18, 2018

stkrwork added 10 commits June 21, 2018 01:50

Added missing includes for optional

be12091

Removed shared pointer from pistache generator

dde9844

Changed Net namespace to Pistache Namespace

770737b

Clean up code and removed unnecessary lines in mustache files

d128153

Removed remaining shared pointer syntax

6c924d7

Code review fixes + updated samples

1f4fb1c

Added const to all model setter functions, and reference to all param…

91e795f

…s in setters that are not primitives

Refactored modelbase

6fb317d

Removed const

3b98e31

Updated samples

38d86e5

stkrwork force-pushed the pistahce_optional_and_api_fix branch from c6c1da4 to 38d86e5 Compare June 20, 2018 23:52

wing328 merged commit 23ab517 into OpenAPITools:master Jun 22, 2018

etherealjoy mentioned this pull request Jul 7, 2018

[C++][Pistache-server] Wrong code generation : prototype does not exist #482

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[C++][Pistache] Fix optional error and wrong function signatures #264

[C++][Pistache] Fix optional error and wrong function signatures #264

stkrwork commented Jun 9, 2018 •

edited

Loading

stkrwork commented Jun 10, 2018

etherealjoy Jun 10, 2018

stkrwork Jun 10, 2018

etherealjoy Jun 10, 2018

stkrwork Jun 10, 2018

etherealjoy Jun 10, 2018

stkrwork Jun 10, 2018

etherealjoy commented Jun 10, 2018 •

edited

Loading

MartinDelille commented Jun 10, 2018

stkrwork commented Jun 10, 2018

MartinDelille commented Jun 10, 2018

stkrwork commented Jun 10, 2018

MartinDelille commented Jun 10, 2018

etherealjoy commented Jun 10, 2018

stkrwork commented Jun 10, 2018

stkrwork commented Jun 10, 2018

CyrilleBenard commented Jun 10, 2018

CyrilleBenard commented Jun 12, 2018

stkrwork commented Jun 12, 2018

wing328 commented Jun 14, 2018

CyrilleBenard commented Jun 14, 2018 •

edited

Loading

stkrwork commented Jun 14, 2018

CyrilleBenard commented Jun 14, 2018 •

edited

Loading

stkrwork commented Jun 14, 2018

wing328 commented Jun 14, 2018

Schrigga commented Jun 17, 2018

wing328 commented Jun 17, 2018

[C++][Pistache] Fix optional error and wrong function signatures #264

[C++][Pistache] Fix optional error and wrong function signatures #264

Conversation

stkrwork commented Jun 9, 2018 • edited Loading

PR checklist

Description of the PR

stkrwork commented Jun 10, 2018

etherealjoy Jun 10, 2018

Choose a reason for hiding this comment

stkrwork Jun 10, 2018

Choose a reason for hiding this comment

etherealjoy Jun 10, 2018

Choose a reason for hiding this comment

stkrwork Jun 10, 2018

Choose a reason for hiding this comment

etherealjoy Jun 10, 2018

Choose a reason for hiding this comment

stkrwork Jun 10, 2018

Choose a reason for hiding this comment

etherealjoy commented Jun 10, 2018 • edited Loading

MartinDelille commented Jun 10, 2018

stkrwork commented Jun 10, 2018

MartinDelille commented Jun 10, 2018

stkrwork commented Jun 10, 2018

MartinDelille commented Jun 10, 2018

etherealjoy commented Jun 10, 2018

stkrwork commented Jun 10, 2018

stkrwork commented Jun 10, 2018

CyrilleBenard commented Jun 10, 2018

CyrilleBenard commented Jun 12, 2018

stkrwork commented Jun 12, 2018

wing328 commented Jun 14, 2018

CyrilleBenard commented Jun 14, 2018 • edited Loading

stkrwork commented Jun 14, 2018

CyrilleBenard commented Jun 14, 2018 • edited Loading

stkrwork commented Jun 14, 2018

wing328 commented Jun 14, 2018

Schrigga commented Jun 17, 2018

wing328 commented Jun 17, 2018

stkrwork commented Jun 9, 2018 •

edited

Loading

etherealjoy commented Jun 10, 2018 •

edited

Loading

CyrilleBenard commented Jun 14, 2018 •

edited

Loading

CyrilleBenard commented Jun 14, 2018 •

edited

Loading