[JavaScript] Generator options and template improvements

[demonfiddler]

Please review and merge the fix and tests for Issue #2258 "[JavaScript] Generator options and template improvements". Many thanks.

[wing328]

@demonfiddler thanks for the PR. Your 2nd and 3rd commits are not linked to your Github account, which means these contributions won't count toward https://github.com/swagger-api/swagger-codegen/graphs/contributors as your contribution.

[wing328]

cc @delenius @xhh

[demonfiddler]

@wing328 thanks for the heads-up regarding my 2nd and 3rd commits. I'm still on the Git/GitHub learning curve and hadn't realised the significance of the commit user name WRT attributions. Actually it's only the 3rd commit that contains my changes - the first two were just pulls from remotes. Is there anything could be done to rectify the contributor attribution?

Also, I spent all day yesterday pulling and rebasing to avoid conflicts, and when I raised this PR it could have been merged without conflict. Now it would seem that someone else has made a conflicting commit. I'm not sure how to proceed...

[wing328]

@demonfiddler for the rebase, you can try to do it or I can do it for you when I merge later.

About fixing the author for commits, would this help? https://gist.github.com/trey/9588090

[demonfiddler]

Thanks for the hints @wing328, I figured out how to do both the rebase and the user name amendment and forced a re-push. Lools like the CI build is now running... UPDATE: CI build succeeded!

[wing328]

👍

[wing328]

@demonfiddler can you please fix the unit test for JS Petstore API client?

When I run the unit test as following, I got 15 errors:

mvn clean && ./bin/javascript-petstore.sh
cd samples/client/petstore/javascript
mvn integration-test

After fixing the test cases for javascript, please do the same for javascript-promise ( samples/client/petstore/javascript-promise)

If you do not have cycle to update the test cases, then we might need to roll back the change for the time being.

(and sorry it was my oversight..)

[demonfiddler]

@wing328 - my sincerest apologies, to be honest I had noticed that there were some JavaScript samples but they seemed to contain source-controlled generated API files - not what I'd expected for an integration test or a sample, so I left them alone.

Leave this with me, I'll sort it out.

[wing328]

@demonfiddler no need to apologize. It's my responsibility to ensure the test cases for Petstore pass before merging and my manual tests didn't cover that (so it's my fault).

Please take your time in updating the test cases.

[demonfiddler]

This will take a while, turns out the ITs won't run on Windows. I've had to set up a dev env in an Ubuntu VM but mvn integration-test (actually npm test) fails 'cos it can't find the node executable (it's nodejs in Debian distros). Anyhow, sorted, now making some progress...

[delenius]

Please also update and run the "promise" version of the integration tests:

mvn clean && ./bin/javascript-promise-petstore.sh
cd samples/client/petstore-promise/javascript
mvn integration-test

(this stuff should really be automated)

[demonfiddler]

Integration test fixes submitted as Pull Request #2411.

[wing328]

@demonfiddler thanks. Tested locally and merged into master.

[cbornet]

@demonfiddler I'm doing some fixes and unit tests on inheritance/composition. Can you explain the difference between "vars" and "allVars" ? They seem to do exactly the same thing (except that you need to use one or the other depending on supportsInheritance which makes a lot of duplicate code in the template (not so much for javascript it seems but for java this is huge))

[demonfiddler]

Hello Christophe,

vars and allVars are identical if the generator does not support inheritance. If the generator does support inheritance, vars lists those fields owned by the class itself whereas allVars is the complete set of fields both owned and inherited (superclass first). You’ll see that in the JavaScript templates both are used – allVars being used in conjunction with the ‘required’ property to determine the constructor parameters. The JavaScript generator can be configured to support inheritance or not. It implements inheritance via idiomatic JavaScript prototype chains and non-inheritance by declaring one property for each of allVars in the class prototype (but no prototype chain). I’m not that familiar with the Java generator but I recall that it didn’t do a great job of inheritance – I half intended to do some clean-up on it myself but never got the time.

I hope this answers your question, please write back if you need more information.

Cheers,

--A

From: Christophe Bornet [mailto:[email protected]]
Sent: 23 August 2016 17:58
To: swagger-api/swagger-codegen [email protected]
Cc: Adrian Price [email protected]; Mention [email protected]
Subject: Re: [swagger-api/swagger-codegen] [JavaScript] Generator options and template improvements (#2396)

@demonfiddler https://github.com/demonfiddler I'm doing some fixes and unit tests on inheritance/composition. Can you explain the difference between "vars" and "allVars" ? They seem to do exactly the same thing (except that you need to use one or the other depending on supportsInheritance which makes a lot of duplicate code in the template (not so much for javascript it seems but for java this is huge))

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #2396 (comment) , or mute the thread https://github.com/notifications/unsubscribe-auth/ANXPu-VMLlEoPyMLeaR13hOxvEyPE3iiks5qiyalgaJpZM4HyPw1 .

This email has been scanned by BullGuard antivirus protection.
For more info visit www.bullguard.com http://www.bullguard.com/tracking.aspx?affiliate=bullguard&buyaffiliate=smtp&url=/

[cbornet]

Oh, I didn't see you were using vars without allVars in other template files !
So that makes sense.
I'm precisely working on inheritance and composition for Java 😄

[cbornet]

Actually, if you also copy the interface properties to vars when supportInheritance is true (vars = own + interfaces but not parent) then it simplifies the model template and it also works well in the other places where you use vars (doc and test).
So I'm still not sure it's useful to separate them...

[cbornet]

Reducing vars to the "own" properties would only be useful for languages that support mixins or some kind of multiple inheritance. For them I think we could introduce a supportsMixins options.