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] Fixed bug for handling optional header parameters #3776

Merged
merged 5 commits into from Sep 15, 2016

Conversation

Projects
None yet
3 participants
@geetikagupta16
Copy link
Contributor

commented Sep 12, 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

Refactored code to handle optional header parameters

geetikagupta16 added some commits Sep 12, 2016

Fixed #3774
Refactored code to handle optional header parameters.
{{/headerParams}}
{{#headerParams}}
{{^required}}
if ({{paramName}} != null) {{paramName}}.map { v => headerParams += "{{baseName}}" -> v.toString }

This comment has been minimized.

Copy link
@wing328

wing328 Sep 12, 2016

Contributor

Can we use 2-space indentation instead?

And what if {{paramName}} (variable) is a non-string? Shall we use {{#isString}} ... {{/isString}} to do the null check only if it's a string?

This comment has been minimized.

Copy link
@clasnake

clasnake Sep 13, 2016

Contributor

Yes we should use 2-space indentation here.

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

@wing328 wing328 added the Issue: Bug label Sep 12, 2016

@geetikagupta16

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2016

null check is done just to make sure that the code won't break, if the user has provided null values when calling the Api.

@wing328

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2016

@clasnake when you've time, please review this change. Thanks!

{{/required}}
{{#required}}
if ({{paramName}} != null) headerParams += "{{baseName}}" -> {{paramName}}.toString
{{/required}}

This comment has been minimized.

Copy link
@clasnake

clasnake Sep 13, 2016

Contributor

Since the type of {{paramName}} is an Option, why do we need to do the null check?

This comment has been minimized.

Copy link
@wing328

wing328 Sep 13, 2016

Contributor

My understanding is that {{paramName}} is an Option if it's an optional parameter.

If it's required, then we still need the null check (if the data type is string)

This comment has been minimized.

Copy link
@clasnake

clasnake Sep 13, 2016

Contributor

Ah I see. But why do we use Option for non-required and raw type for required params? Required or not can be achieved by whether providing default value or not. It doesn't matter whether we use Option or not. It's orthogonal.

This comment has been minimized.

Copy link
@wing328

wing328 Sep 13, 2016

Contributor

I do not know the reason (I was not the one who created the async-scala generator).

scala generator (API client) also follows similar convention: Option for non-required and raw type for required.

If Option should be used for both required and non-required parameters, I welcome the (breaking) change as long as it conforms to Scala style guide or best practices.

This comment has been minimized.

Copy link
@clasnake

clasnake Sep 13, 2016

Contributor

Yes, basically we will never see null in Scala code. And actually that's the reason why we have Option.

@clasnake

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2016

@wing328 @geetikagupta16 Just finished review. We don't need any null check here because {{paramName}} is an Option. Use pattern matching instead and do the operation if it matches Some(xxx).

@geetikagupta16

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2016

@clasnake @wing328 Thanks for the review.
I have made the required changes. Please have a look at it.

@wing328 wing328 merged commit 1e4f30e into swagger-api:master Sep 15, 2016

3 checks passed

Shippable Run 440 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 15, 2016

@geetikagupta16 thanks for the contribution. PR merged into master.

In the future, we may merge the async-scala generator into the scala generator so that the Scala API client will provide both async and synchronous methods (similar to what we've done for other API clients, e.g. C#, Ruby, Python, etc)

@geetikagupta16

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2016

@wing328 Thanks for merging PR

@wing328 wing328 changed the title Fixed bug for handling optional header parameters [Scala] Fixed bug for handling optional header parameters 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.