[Scala] Default Option parameters to None#6538
Conversation
|
@gmarz thank for the PR. |
| {{/allParams}} * @return {{#returnType}}{{{returnType}}}{{/returnType}}{{^returnType}}void{{/returnType}} | ||
| */ | ||
| def {{operationId}}({{#allParams}}{{paramName}}: {{#required}}{{dataType}}{{#defaultValue}} /* = {{{defaultValue}}}*/{{/defaultValue}}{{/required}}{{^required}}Option[{{dataType}}]{{#defaultValue}} /* = {{{defaultValue}}}*/{{/defaultValue}}{{^defaultValue}} = None{{/defaultValue}}{{/required}}{{#hasMore}}, {{/hasMore}}{{/allParams}}){{#returnType}}: Option[{{returnType}}]{{/returnType}} = { | ||
| def {{operationId}}({{#allParams}}{{paramName}}: {{#required}}{{dataType}}{{#defaultValue}} /* = {{{defaultValue}}}*/{{/defaultValue}}{{/required}}{{^required}}Option[{{dataType}}]{{#defaultValue}} = None /* = {{{defaultValue}}}*/{{/defaultValue}}{{^defaultValue}} = None{{/defaultValue}}{{/required}}{{#hasMore}}, {{/hasMore}}{{/allParams}}){{#returnType}}: Option[{{returnType}}]{{/returnType}} = { |
There was a problem hiding this comment.
@gmarz it's already default to None if defaultValue is not provided:
{{^defaultValue}} = None{{/defaultValue}}
I think the following should be changed from
/* = {{{defaultValue}}}*/
to
= {{{defaultValue}}}
instead to use the default value provided in the spec.
(I do not recall why it's commented. Maybe it's not working for all default values)
There was a problem hiding this comment.
@gmarz can you tell me what are the steps to reproduce the error, and I will test it.
There was a problem hiding this comment.
@wing328 sorry for the slow reply...
(I do not recall why it's commented. Maybe it's not working for all default values)
Yea, I'm not sure. Maybe because it needs to be wrapped in Some...I think the current template will try and do Option[Boolean] = false which isn't valid?
@ramzimaalej there is no error/issue here. This change just defaults Option types to None for convenience.
There was a problem hiding this comment.
@gmarz do you mind resolving the conflicts and I will take a look.
There was a problem hiding this comment.
@gmarz I think the current implementation should be able to meet your requirement:
/* = {{{defaultValue}}}*/{{/defaultValue}}{{^defaultValue}} = None{{/defaultValue}}
which is default to None if defaultValue is not provided.
In your spec, do you have the defaultValue set to something?
There was a problem hiding this comment.
@gmarz do you mind double checking if current version solves the problem or not. if the problem is not solved, please create a ticket (if it does not exists) and attach the swagger-doc you used to reproduce the issue
There was a problem hiding this comment.
The default value is commented out anyway so it should not be a problem merging this PR into master if the spec contains default value. I'll create a separate "issue" to track the defaultValue enhancement to Scala API client.
There was a problem hiding this comment.
@gmarz @wing328 @ramzimaalej the only issue I see with the previous default value is that if someone were to say defaultValue = null it would fail as Some(null) defines a value of null, which is unexpected. The optional value should be Option({{defaultValue}}) to account for this edge case, as Option(null) becomes None and Option(nonNull) becomes Some(nonNull).
gmarz
force-pushed
the
fix/scala-default-params
branch
2 times, most recently
from
ee6af15 to
d219577
Compare
|
@wing328 LGTM |
5 participants
PR checklist
./bin/to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.shand./bin/security/{LANG}-petstore.shif 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\.3.0.0branch for breaking (non-backward compatible) changes.Description of the PR
Defaults
Optionparameters toNonewhich makes method calls and models less verbose.