fix some rest js client problems #1566
Conversation
deviator
changed the title
|
ping @s-ludwig |
|
The Travis failure is due to a flaky test - I've restarted the test run now and will remove that specific test. Changes look good, except for the mentioned if/else swap. Maybe the three additional parameters could be grouped inside of a |
|
@s-ludwig it's ok? |
|
Looks good to merge, thanks! I'll just make the |
| } else { | ||
| output.formattedWrite(" var url = %s;\n", Json(concatURL(intf.baseURL, route.pattern))); | ||
| auto rpn = route.parameters |
There was a problem hiding this comment.
Stupid new review feature has eaten my earlier comment...
So this part is strange, because the upper part of the if statement is supposed to be doing what you did in the else part. Maybe there is an issue with route.hasPathPlaceholders.
There was a problem hiding this comment.
I was look to this part, but was confusing by this and I don't touch this, because I think change in this part can have more complex effects. Is this a bug or only first id parameter can be a placeholders by design? If it by design it's doubtful (or I don't understand somethings).
|
ping @s-ludwig |
| else output.formattedWrite("encodeURIComponent(toRestString(%s))", p.text); | ||
| fout.put(" + "); | ||
| if (!p.isParameter) fout.serializeToJson(p.text); | ||
| else fout.formattedWrite("encodeURIComponent(%s)", p.text); |
There was a problem hiding this comment.
I believe the toRestString must conceptually stay here, even if the function itself is currently not implemented. At some point it must be made equal to the one defined in the D code base.
There was a problem hiding this comment.
I implement this function as JSON.stringify, because if you want use objects as a parameter of get request you get object.Object in params instead {"x":2, "y":3} for example. If use it in this case result url has quotes symbols. Maybe need add toQueryParameter for get request?
There was a problem hiding this comment.
The correct semantics are these:
string toRestString(Json value)
{
switch (value.type) {
default: return value.toString();
case Json.Type.Bool: return value.get!bool ? "true" : "false";
case Json.Type.Int: return to!string(value.get!long);
case Json.Type.Float: return to!string(value.get!double);
case Json.Type.String: return value.get!string;
}
}
I.e. mostly strings are special cased to omit the quotes. Otherwise it matches JSON.stringify.
There was a problem hiding this comment.
All of calls toRestString returns value to encodeURIComponent, maybe encodeURIComponent must calls inside of toRestString?
|
I beginner in js and can make a mistake, but on my examples it's code works. |
|
Looks good, except that arrays also need to be stringified (I'd reverse the condition and stringify everything that is not a string). |
|
Here arrays |
|
Oh, alright, didn't expect that! I'll merge then once Travis has finished. Thanks for all the quick follow-up changes! |
fix #1565
some problem from #1564 fix also