Serialize body parameter value #1006 #1012
Conversation
src/execute.js
Outdated
| @@ -108,12 +108,16 @@ export function buildRequest({ | |||
| value = parameters | |||
| } | |||
|
|
|||
| value = parameter && parameter.name && parameters[parameter.name] | |||
| value = (parameter && parameter.name && parameters[parameter.name]) || value | |||
There was a problem hiding this comment.
The if statement above was dead code because of this line. This change allows you to use the parameters object as the value of a body parameter (parameters = {mydata: ''}) instead of having to have parameters be: {body_param_name: {mydata: ''}}
There was a problem hiding this comment.
If the if above is dead code, should it be removed? And if it's removed, what value does value have before this line?
Also, paramater can't be null, can it? If yes, no need to check for parameter.
Should rewrite this thing to be consistent with the stuff below.
if (condition) {
value = ...
}
There was a problem hiding this comment.
The if statement above is not dead code anymore. I can refactor it a bit to make it more readable. Parameter could technically be null since it is just iterating through an array though there is code above this line that would throw an error if parameter is null. Also, that code checking parameter was there before my change so I assumed there is a reason it is there. I can remove it since it seems pointless
src/execute.js
Outdated
| @@ -108,12 +108,16 @@ export function buildRequest({ | |||
| value = parameters | |||
| } | |||
|
|
|||
| value = parameter && parameter.name && parameters[parameter.name] | |||
| value = (parameter && parameter.name && parameters[parameter.name]) || value | |||
There was a problem hiding this comment.
If the if above is dead code, should it be removed? And if it's removed, what value does value have before this line?
Also, paramater can't be null, can it? If yes, no need to check for parameter.
Should rewrite this thing to be consistent with the stuff below.
if (condition) {
value = ...
}
test/execute.js
Outdated
| }) | ||
| }) | ||
| }) | ||
|
|
||
| it('should pass through the parameters object as the value if parameter is in the body', function () { |
There was a problem hiding this comment.
I don't understand what "pass through" here means. And what value does this test add compared to existing one such as "should build a request for all given field"?
There was a problem hiding this comment.
Yeah, that is a little confusing. I mean that the whole parameters object will be used as the parameter value instead of parameters[parameter.name]. I'll clarify the wording. To your second question, the difference in this test is the parameters object in the buildRequest call below.
In the other tests it is:
parameters: { ..., body: { name: 'johny', id: '123' }, }
in this test it is:
parameters: { name: 'johny', id: '123' }
This allows you to call operations like client.apis.default.operation({name: 'johny', id: '123'}) if there is a body parameter.
| }) | ||
| }) | ||
| }) | ||
|
|
||
| it('should use the whole parameters object as the parameter value if parameter is in the body', function () { |
There was a problem hiding this comment.
@webron this is about how the spec works. I'm not sure if what is proposed is valid. Please check.
There was a problem hiding this comment.
I don't really know how to read the test so I don't understand what it does.
There was a problem hiding this comment.
See #1006.
Also, if an api endpoint only has one parameter, what do you think about allowing you to call the operation with the interface operation(value) instead of operation({param_name: value})? If an operation only has one parameter, you could check if the argument passed to the operation is an object with a property matching the parameter name (existing behavior) and if not then just use the passed in argument as the value for the parameter (after validating the type). Are there problems with that approach or does that make it too complicated?
Maybe @fehguy can chime in.
There was a problem hiding this comment.
Doesn't sound like a spec issue, but rather a simplified interface for call execution. It does make sense to me, but would rather hear from @fehguy as well.
|
Seems obsolete compared to #1034. |
This fixes the issue in #1006. There was a test case for this but it wasn't testing what it said it was testing.