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.

Sign up for GitHub

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

Jump to bottom

Remove dependency on BindingContextFactory #909

Merged
merged 1 commit into from
Oct 29, 2018
Merged

Remove dependency on BindingContextFactory #909

merged 1 commit into from
Oct 29, 2018

Conversation

osana
Copy link
Contributor

@osana osana commented Oct 29, 2018

closes #852

@osana osana mentioned this pull request Oct 29, 2018
Closed
@danesfeder danesfeder self-requested a review October 29, 2018 17:44
danesfeder
danesfeder approved these changes Oct 29, 2018
View reviewed changes
Copy link
Contributor

@danesfeder danesfeder left a comment

Choose a reason for hiding this comment

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

Thanks for running with this @osana!

Guardiola31337
Guardiola31337 approved these changes Oct 29, 2018
View reviewed changes
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

Looks good to me @osana 🚢

@Guardiola31337
Copy link
Contributor

As commented in #908 (comment)

After fixing this I believe that Proguard rule won't be necessary anymore so we'll need to remove it from the different projects in which is included (I'll remark this in the PR).

Let's make sure that we remove the Proguard rule from the Maps and Navigation SDKs (I believe these are the only ones including it) when OP gets released @danesfeder @tobrun

@osana osana merged commit 73fd5ca into master Oct 29, 2018
@osana osana deleted the osana-852 branch October 29, 2018 18:15
zugaldia
zugaldia reviewed Oct 29, 2018
View reviewed changes
services-directions/src/main/java/com/mapbox/api/directions/v5/MapboxDirections.java
@@ -184,14 +184,14 @@ private void errorDidOccur(@Nullable Callback<DirectionsResponse> callback,
Converter<ResponseBody, DirectionsError> errorConverter =
getRetrofit().responseBodyConverter(DirectionsError.class, new Annotation[0]);
if (callback == null) {
BindingContextFactory.LOGGER.log(
Logger.getLogger(MapboxDirections.class.getName()).log(
Copy link
Member

Choose a reason for hiding this comment

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

Instead of duplicating Logger.getLogger(MapboxDirections.class.getName()) several times, could you extract it as a local private class-level variable instead?

@zugaldia
Copy link
Member

zugaldia commented Oct 29, 2018 •
edited
Loading

Let's make sure that we remove the Proguard rule from the Maps and Navigation SDKs (I believe these are the only ones including it) when OP gets released @danesfeder @tobrun

@osana Could you ticket that out on https://github.com/mapbox/mapbox-gl-native?

@Guardiola31337 Guardiola31337 mentioned this pull request Oct 30, 2018
Closed
@osana osana mentioned this pull request Nov 12, 2018
Closed
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zugaldia zugaldia zugaldia left review comments

@Guardiola31337 Guardiola31337 Guardiola31337 approved these changes

@danesfeder danesfeder danesfeder approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Problem with BindingContextFactory dependency and use
4 participants
@osana @Guardiola31337 @zugaldia @danesfeder