Skip to content

Conversation

@langsmith
Copy link

Resolves #996 by adding a required /mapbox/ slug to the Isochrone API service's Retrofit request URL. Based on my demo app refactoring work in mapbox/mapbox-android-demo#1029, the #996 bug renders the wrapper unusable.

The API's docs

Screen Shot 2019-04-11 at 3 57 45 PM

Totally fine if this is the wrong way to fix the bug. Figured I'd open something to get the conversation started.

@langsmith
Copy link
Author

Actually, I'm now seeing how other navigation API wrappers in the repo, handle the user agent. Will adjust the isochrone support accordingly

@langsmith langsmith changed the title added /mapbox/ to Isochrone request retrofit call URL Adding userAgent parameter to Isochrone request retrofit call URL Apr 12, 2019
@osana
Copy link
Contributor

osana commented Apr 12, 2019

@langsmith Technically speaking this becomes a 5.0.0 release because the API changed and it is not backward compatible. I am surprised the tests did not catch this? I guess because a response was mocked. Did your sample code work?

Also, I think the way to fix this without making a major release would be to add the new method (not modify the existing one). One old one can be deprecated and we will remove it when 5.0.0 is done.

@langsmith langsmith force-pushed the ls-isochrone-fix branch 2 times, most recently from a872637 to 7fadb5a Compare April 12, 2019 17:59
@langsmith
Copy link
Author

I am surprised the tests did not catch this? I guess because a response was mocked. Did your sample code work?

Yea, sample code worked. I think we didn't catch this because I put the /mapbox/ slug in the Makefile isochrone fixture URLs (https://github.com/mapbox/mapbox-java/blob/master/Makefile#L199).

Also, I think the way to fix this without making a major release would be to add the new method (not modify the existing one). One old one can be deprecated and we will remove it when 5.0.0 is done.

Sounds 💯 . I've pushed and rebased this fix. Added a new getCall() method which has the user parameter. Also added needed deprecated annotations to the previous getCall() method.

@langsmith
Copy link
Author

Going to test this work by using a Java services SNAPSHOT in the demo app isochrone refactor pr mapbox/mapbox-android-demo#1029

@langsmith langsmith merged commit 5a1e4e9 into master Apr 12, 2019
@langsmith langsmith deleted the ls-isochrone-fix branch April 12, 2019 23:11
@osana osana added this to the v4.7.0 milestone Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Isochrone URL incorrectly built

2 participants