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

[Scala] Added functionality to handle optional parameters for Scala #3683

Merged
merged 4 commits into from Sep 1, 2016

Conversation

Projects
None yet
2 participants
@jyotsnakaran
Copy link
Contributor

commented Aug 31, 2016

PR checklist

  • Read the contribution guildelines.
  • Ran the shell/batch script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates)
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

Added functionality to handle optional parameters for Scala
#3665

jyotsnakaran added some commits Aug 31, 2016

@wing328

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2016

CI (travis) failed. Please check the result: https://travis-ci.org/swagger-api/swagger-codegen/jobs/156459865

You can also repeat the issue by running sbt test locally under the Scala Petstore folder.

jyotsnakaran added some commits Aug 31, 2016

jyotsnakaran
1. Updated api.mustache to handle optional thing with headers and fil…
…e parameters

2. Generated petstore sample for the above changes
jyotsnakaran
@jyotsnakaran

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2016

Previously I missed running those tests for scala petstore. After running that I realized I was missing some cases to be handled for optional params.
I have made the required changes and checked in the code. Please review it.

@jyotsnakaran jyotsnakaran changed the title Feature/issue#3665 Added functionality to handle optional parameters for Scala Aug 31, 2016

{{#queryParams}}if(String.valueOf({{paramName}}) != "null") queryParams += "{{baseName}}" -> {{paramName}}.toString
{{#queryParams}}
{{#required}}
if(String.valueOf({{paramName}}) != "null") queryParams += "{{baseName}}" -> {{paramName}}.toString

This comment has been minimized.

Copy link
@wing328

wing328 Aug 31, 2016

Contributor

I don't think that's the correct way to do a null check in scala

It would be nice to additionally do the following:

  • add an else clause to throw an exception (e.g. java.lang.IllegalArgumentException)
  • add the same null check on required parameter for other type of parameters (e.g. header, form, body)

You may want to look at the Java API client on how it validates required parameters.

@jyotsnakaran

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2016

Actually, I was just focusing on optional parameters. For required ones, the code is same as before, I have just made changes for the optional params.

And the check for null parameters is already there on line no: 51 which throwing exception for missing required parameters.

Please have a look for the same and let me know.

@wing328

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2016

Understood (I didn't mean it's your fault, just want to point out something not right during my review).

I'll take another look and let you know if I've further question.

@jyotsnakaran

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2016

No, that's completely fine. :) I was just letting you know about the already existing null parameter handling in the code, so that you can have a look over it.
Let me know, if you need anything from my side.

@wing328 wing328 added this to the v2.2.2 milestone Sep 1, 2016

@wing328 wing328 merged commit 062e6fc into swagger-api:master Sep 1, 2016

3 checks passed

Shippable Run 280 status is SUCCESS.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@wing328

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2016

PR merged into master. Thanks for the contribution.

@jyotsnakaran

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2016

@wing328 Thanks for merging.

acramatte added a commit to comerge/swagger-codegen that referenced this pull request Sep 5, 2016

Merge branch 'master' of https://github.com/swagger-api/swagger-codegen
* 'master' of https://github.com/swagger-api/swagger-codegen:
  Issue swagger-api#3687 silence resty logging [Go] (swagger-api#3705)
  update ruby regular expression to use \A
  [Ruby] Fix ambiguous regex (swagger-api#3716)
  [PHP] Corrected PHPDoc type declarations (swagger-api#3710)
  Python collection formatting fixes/support (swagger-api#3697)
  php: Fix syntax error when pattern contains a single quote
  ruby: Fix syntax error when pattern contains a single quote
  [Scalatra] replace {} with : in scalatra path (swagger-api#3694)
  java: Javadoc fixes
  Spelling fixes
  Added 'modelPropertyNaming' option for Scala (swagger-api#3685)
  Added functionality to handle optional parameters for Scala (swagger-api#3683)
  Python fixes (swagger-api#3689)

@wing328 wing328 changed the title Added functionality to handle optional parameters for Scala [Scala] Added functionality to handle optional parameters for Scala Feb 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.