Skip to content

Conversation

richardjrossiii
Copy link
Contributor

Moves server from ParseObject to ParseRestCommand, and allows it to be
changed via Builder.server("myUrl").

@facebook-github-bot
Copy link

By analyzing the blame information on this pull request, we identified @grantland, @richardjrossiii and @wangmengyan95 to be potential reviewers.

@@ -8,6 +8,8 @@
*/
package com.parse;

import android.net.Uri;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use java.net.URL instead

@richardjrossiii
Copy link
Contributor Author

Updated PR. Take another look 😄

@facebook-github-bot
Copy link

@richardjrossiii updated the pull request.

@@ -63,7 +63,7 @@
for (ParseRESTObjectCommand command : commands) {
JSONObject requestParameters = new JSONObject();
requestParameters.put("method", command.method.toString());
requestParameters.put("path", String.format("/1/%s", command.httpPath));
requestParameters.put("path", String.format("%s/%s", server.getPath(), command.httpPath));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing slash issue would still appear in batch encoding, I don't have a great solution for that however, ATM.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an ability to ammend path componenets to path when building a URI?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this prevented in iOS? Just different internal URL logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grantland yep, simply via different url logic.

We could solve this by doing new URL(server, command.httpPath).getPath(), though that potentially creates a lot of temporary objects in the interim.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly either way

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are doing this for ParseRESTCommand, though

@facebook-github-bot
Copy link

@richardjrossiii updated the pull request.

1 similar comment
@facebook-github-bot
Copy link

@richardjrossiii updated the pull request.

public Builder server(String server) {
this.server = server;
return this;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind updating the tests to cover this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@facebook-github-bot
Copy link

@richardjrossiii updated the pull request.

@grantland
Copy link
Contributor

LGTM, squash and feel free to merge!

Moves `server` from ParseObject to ParseRestCommand, and allows it to be
changed via `Builder.server("myUrl")`.
@richardjrossiii richardjrossiii force-pushed the richardross.configuration.serverurl branch from fe0507b to 01b0638 Compare January 5, 2016 23:23
richardjrossiii added a commit that referenced this pull request Jan 6, 2016
…serverurl

Add configurable serverURL to Parse.Configuration.
@richardjrossiii richardjrossiii merged commit 2dd0095 into master Jan 6, 2016
@richardjrossiii richardjrossiii deleted the richardross.configuration.serverurl branch January 6, 2016 00:17
@grantland grantland modified the milestone: 1.13.0 Jan 12, 2016
This was referenced Jan 12, 2016
@facebook-github-bot
Copy link

@richardjrossiii updated the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants