Skip to content
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

[android] - make changing base url a part of public API. #245

Merged
merged 1 commit into from
Dec 20, 2016

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Dec 20, 2016

Closes #244.

@tobrun tobrun self-assigned this Dec 20, 2016
@tobrun tobrun requested a review from cammace December 20, 2016 10:18
@mention-bot
Copy link

@tobrun, thanks for your PR! By analyzing the history of the files in this pull request, we identified @cammace, @ivovandongen and @zugaldia to be potential reviewers.

* Get the base url of the API.
*
* @return the base url used as endpoint.
*/
Copy link

Choose a reason for hiding this comment

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

Missing @SInCE 2.0.0 tag


public abstract MapboxBuilder setAccessToken(String accessToken);

public abstract String getAccessToken();

/**
* Set the base url of the API.
*/
Copy link

Choose a reason for hiding this comment

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

Missing @SInCE 2.0.0 and @param tags

@@ -458,6 +442,11 @@ public MapboxDirections build() throws ServicesException {
return new MapboxDirections(this);
}

@Override
Copy link

Choose a reason for hiding this comment

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

No need to expose this in Directions v4, it has been deprecated.

@@ -458,6 +444,11 @@ public Builder setClientAppName(String appName) {
return this;
}

public Builder setBaseUrl(String baseUrl) {
Copy link

Choose a reason for hiding this comment

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

Javadoc? Looks like we are also missing javadoc for the method above: setClientAppName

@@ -277,6 +263,12 @@ public Builder setClientAppName(String appName) {
return this;
}

@Override
Copy link

Choose a reason for hiding this comment

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

Javadoc

@@ -448,6 +434,12 @@ public Builder setClientAppName(String appName) {
return this;
}

@Override
Copy link

Choose a reason for hiding this comment

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

Javadoc

@@ -318,6 +303,12 @@ public Builder setClientAppName(String appName) {
return this;
}

@Override
Copy link

Choose a reason for hiding this comment

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

Javadoc

@@ -375,6 +373,12 @@ public Builder setClientAppName(String appName) {
return this;
}

@Override
Copy link

Choose a reason for hiding this comment

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

Javadoc

@cammace
Copy link

cammace commented Dec 20, 2016

Also, build is failing since we are now using 5.0.0 snapshot correct?

@tobrun
Copy link
Member Author

tobrun commented Dec 20, 2016

thanks for the review, the build is failing since 4.2.0 snapshot doesn't exist anymore -> that is why I created #247. IMO we should avoid using snapshots between projects except for doing some testing.

@tobrun tobrun force-pushed the 244-make-changing-base-url-public-api branch from 6e357ea to b0432db Compare December 20, 2016 19:55
Copy link

@cammace cammace left a comment

Choose a reason for hiding this comment

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

👌

@tobrun tobrun merged commit 4e710b1 into master Dec 20, 2016
@tobrun tobrun deleted the 244-make-changing-base-url-public-api branch December 20, 2016 20:20
@zugaldia zugaldia mentioned this pull request Feb 10, 2017
9 tasks
@zugaldia zugaldia mentioned this pull request Feb 22, 2017
9 tasks
@zugaldia zugaldia mentioned this pull request Mar 9, 2017
9 tasks
@zugaldia zugaldia mentioned this pull request Mar 17, 2017
9 tasks
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.

3 participants