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

fix some rest js client problems #1566

Merged
merged 12 commits into from Sep 23, 2016

Conversation

Projects
None yet
3 participants
@deviator
Contributor

deviator commented Sep 18, 2016

fix #1565
some problem from #1564 fix also

@deviator deviator changed the title from fix forgotten template parameters to fix some rest js client problems Sep 18, 2016

@deviator

This comment has been minimized.

Show comment
Hide comment
@deviator

deviator Sep 18, 2016

Contributor

ping @s-ludwig

Contributor

deviator commented Sep 18, 2016

ping @s-ludwig

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 20, 2016

Member

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 JSRestInterfaceSettings class - it seems like more options could come in handy later on.

Member

s-ludwig commented Sep 20, 2016

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 JSRestInterfaceSettings class - it seems like more options could come in handy later on.

@deviator

This comment has been minimized.

Show comment
Hide comment
@deviator

deviator Sep 21, 2016

Contributor

@s-ludwig it's ok?

Contributor

deviator commented Sep 21, 2016

@s-ludwig it's ok?

@WowVeryLogin

This comment has been minimized.

Show comment
Hide comment
@WowVeryLogin

WowVeryLogin Sep 22, 2016

Ping @s-ludwig I've encountred bug, described in issue #1564 and it's very important for owr company to continue to work on project, so pls fix this stuff as soon as you can

WowVeryLogin commented Sep 22, 2016

Ping @s-ludwig I've encountred bug, described in issue #1564 and it's very important for owr company to continue to work on project, so pls fix this stuff as soon as you can

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 22, 2016

Member

Looks good to merge, thanks! I'll just make the parent parameter private for now/remove it from the settings class, so that it doesn't become a burden w.r.t. backwards compatibility.

Member

s-ludwig commented Sep 22, 2016

Looks good to merge, thanks! I'll just make the parent parameter private for now/remove it from the settings class, so that it doesn't become a burden w.r.t. backwards compatibility.

Show outdated Hide outdated source/vibe/web/internal/rest/jsclient.d
} else {
output.formattedWrite(" var url = %s;\n", Json(concatURL(intf.baseURL, route.pattern)));
auto rpn = route.parameters

This comment has been minimized.

@s-ludwig

s-ludwig Sep 22, 2016

Member

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.

@s-ludwig

s-ludwig Sep 22, 2016

Member

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.

This comment has been minimized.

@deviator

deviator Sep 22, 2016

Contributor

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).

@deviator

deviator Sep 22, 2016

Contributor

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).

This comment has been minimized.

@s-ludwig

s-ludwig Sep 23, 2016

Member

That was indeed broken. I've committed a fix for the hasPathPlaceholders computation: f23d774
And also one for the JS URL computation: 10e37ed

Can you remove the changes in the else block from the PR and rebase?

@s-ludwig

s-ludwig Sep 23, 2016

Member

That was indeed broken. I've committed a fix for the hasPathPlaceholders computation: f23d774
And also one for the JS URL computation: 10e37ed

Can you remove the changes in the else block from the PR and rebase?

@deviator

This comment has been minimized.

Show comment
Hide comment
@deviator

deviator Sep 23, 2016

Contributor

ping @s-ludwig

Contributor

deviator commented Sep 23, 2016

ping @s-ludwig

Show outdated Hide outdated source/vibe/web/internal/rest/jsclient.d
else output.formattedWrite("encodeURIComponent(toRestString(%s))", p.text);
fout.put(" + ");
if (!p.isParameter) fout.serializeToJson(p.text);
else fout.formattedWrite("encodeURIComponent(%s)", p.text);

This comment has been minimized.

@s-ludwig

s-ludwig Sep 23, 2016

Member

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.

@s-ludwig

s-ludwig Sep 23, 2016

Member

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.

This comment has been minimized.

@deviator

deviator Sep 23, 2016

Contributor

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?

@deviator

deviator Sep 23, 2016

Contributor

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?

This comment has been minimized.

@s-ludwig

s-ludwig Sep 23, 2016

Member

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.

@s-ludwig

s-ludwig Sep 23, 2016

Member

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.

This comment has been minimized.

@deviator

deviator Sep 23, 2016

Contributor

All of calls toRestString returns value to encodeURIComponent, maybe encodeURIComponent must calls inside of toRestString?

@deviator

deviator Sep 23, 2016

Contributor

All of calls toRestString returns value to encodeURIComponent, maybe encodeURIComponent must calls inside of toRestString?

This comment has been minimized.

@s-ludwig

s-ludwig Sep 23, 2016

Member

That could indeed be done.

@s-ludwig

s-ludwig Sep 23, 2016

Member

That could indeed be done.

@deviator

This comment has been minimized.

Show comment
Hide comment
@deviator

deviator Sep 23, 2016

Contributor

I beginner in js and can make a mistake, but on my examples it's code works.

Contributor

deviator commented Sep 23, 2016

I beginner in js and can make a mistake, but on my examples it's code works.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 23, 2016

Member

Looks good, except that arrays also need to be stringified (I'd reverse the condition and stringify everything that is not a string).

Member

s-ludwig commented Sep 23, 2016

Looks good, except that arrays also need to be stringified (I'd reverse the condition and stringify everything that is not a string).

@deviator

This comment has been minimized.

Show comment
Hide comment
@deviator

deviator Sep 23, 2016

Contributor

Here arrays typeof is "object"

Contributor

deviator commented Sep 23, 2016

Here arrays typeof is "object"

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 23, 2016

Member

Oh, alright, didn't expect that! I'll merge then once Travis has finished. Thanks for all the quick follow-up changes!

Member

s-ludwig commented Sep 23, 2016

Oh, alright, didn't expect that! I'll merge then once Travis has finished. Thanks for all the quick follow-up changes!

@s-ludwig s-ludwig merged commit e5e557a into vibe-d:master Sep 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment