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

Add usePromise parameter to JS client #2072

Merged
merged 13 commits into from
Feb 9, 2016
Merged

Conversation

delenius
Copy link
Contributor

@delenius delenius commented Feb 8, 2016

This adds a "usePromise" boolean parameter to the JS client generator (disabled by default).

Fixes #2014.
For some discussion on the benefits of using Promises, see http://bluebirdjs.com/docs/why-promises.html

Note: I have not added any unit tests for this feature, because it produces a different client API, so the existing PetStore tests will not work. I could add:

samples/client/javascript/test/api/PetApiPromiseTest.js
samples/client/javascript/test/api/ApiClientPromiseTest.js

and furthermore, modify bin/javascript-petstore.sh to produce a promise version of the test client.

Let me know if you want me to do that, or create tests in any other way.

In the meantime, consider this addition experimental. It does not impact existing functionality since it has to be explicitly enabled using --additional-properties usePromises=true

@wing328
Copy link
Contributor

wing328 commented Feb 9, 2016

@delenius thanks for adding the support for Promises.

Instead of modifying bin/javascript-petstore.sh, I would suggest adding bin/javascript-promise-petstore.sh and output the JS client with promise in samples under samples/client/petstore/javascript-promise. Then you can add test cases in that directory.

If it's not too much work, I would suggest using CLI option (instead of --additional-properties) for usePromises

@delenius
Copy link
Contributor Author

delenius commented Feb 9, 2016

@wing328, can you point me to how to add a CLI option instead of --additional-properties, for a language-specific option? I don't want to pollute the generic set of options.

@delenius
Copy link
Contributor Author

delenius commented Feb 9, 2016

Tests added as discussed above, though some tests are not implemented:

  1. Can not check values in the request object generated inside ApiClient, because it is not returned (a Promise is returned instead).
  2. Can not check response headers, because they are not returned in the Promise version -- only the data is returned.

@wing328
Copy link
Contributor

wing328 commented Feb 9, 2016

would this help #1851 ?

@delenius
Copy link
Contributor Author

delenius commented Feb 9, 2016

That looks like what I did, except that there is no io/swagger/codegen/options dir, where that OptionProvider goes. How is the parameter in that commit passed on the command line? Maybe the same style already works in my PR.

@wing328
Copy link
Contributor

wing328 commented Feb 9, 2016

Sorry I missed the lines about adding USE_PROMISES to CLIOption.

@wing328
Copy link
Contributor

wing328 commented Feb 9, 2016

Btw, I saw commits from others in the commit tabs. Is that due to git rebase master?

@delenius
Copy link
Contributor Author

delenius commented Feb 9, 2016

Oh, yeah I was confused by that too. Yes, probably. I was trying to integrate the latest master branch into my feature branch...

@@ -96,6 +82,8 @@ public JavascriptClientCodegen() {
"version of the project (Default: using info.version or \"1.0.0\")"));
cliOptions.add(new CliOption(PROJECT_LICENSE_NAME,
"name of the license the project uses (Default: using info.license.name)"));
cliOptions.add(new CliOption(USE_PROMISES,
"use Promises as return values from the client API, instead of superagent callbacks (Default: false)"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of putting the default value in the description, you can use .defaultValue instead. Here is an example:

cliOptions.add(new CliOption(MODULE_NAME, "Perl module name (convention: CamelCase or Long::Module).").defaultValue("SwaggerClient"));

@wing328
Copy link
Contributor

wing328 commented Feb 9, 2016

FYI. I usually rebase my branch (e.g. fix_something) as follows:

git checkout master
git pull upstream master 
git checkout fix_something
git rebase master

Assuming upstream is a remote defined as https://github.com/swagger-api/swagger-codegen.git

@delenius
Copy link
Contributor Author

delenius commented Feb 9, 2016

@wing328, yeah that's how I did my rebase too, except I think I did a git push (to origin, i.e. my fork), after git pull, which seems to have complicated things.

I fixed the defaultValue issue. Note that there are other default values in the same file that use the "manual" style. I didn't want to touch those though.

@wing328
Copy link
Contributor

wing328 commented Feb 9, 2016

@delenius thanks for the update. We can clean those up later.

I also left you a comment about wildcard import.

@delenius
Copy link
Contributor Author

delenius commented Feb 9, 2016

I for sure did not change those imports (it's because other people's commits are listed with my PR for some reason). I agree on not using wildcards, and I typically use IDE auto-imports which does not generate wildcards. But I can try to fix them anyway.

@wing328
Copy link
Contributor

wing328 commented Feb 9, 2016

Yes, you're right. @fehguy did that. No worry then.

I'll review your PR and approve shortly.

@delenius
Copy link
Contributor Author

delenius commented Feb 9, 2016

No worries, I already fixed the imports.

@wing328
Copy link
Contributor

wing328 commented Feb 9, 2016

👍

@delenius
Copy link
Contributor Author

delenius commented Feb 9, 2016

@wing328, regarding the other people's commits showing up: Is it because I messed up my rebase somehow? Or does that always happen after a rebase? I'd like to not repeat the same mistake in the future.

@wing328
Copy link
Contributor

wing328 commented Feb 9, 2016

It never happens to me when I perform a rebase. I've seen others doing that due to a bad rebase.

wing328 added a commit that referenced this pull request Feb 9, 2016
Add usePromise parameter to JS client
@wing328 wing328 merged commit 95c033c into swagger-api:master Feb 9, 2016
@wing328
Copy link
Contributor

wing328 commented Feb 9, 2016

PR merged. Thanks again for the contribution 🍻

@delenius delenius deleted the promises branch February 11, 2016 01:31
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.

3 participants