Refactor RouteOptions.toUrl

[korshaknn]

closes #1313

can't usejava.net.URL case it doesn't have handy methods to build URL, so use okhttp HttpUrl

also some symbols were encoded:
; -> %3B
, -> %2C
it shouldn't be an issue, right?

[Guardiola31337]

Is this change bringing the possibility to pass either https://mapbox.com/ or https://mapbox.com as baseUrl and should work fine? I'm asking because

mapbox-java/services-directions-models/src/main/java/com/mapbox/api/directions/v5/DirectionsCriteria.java

Line 25 in 7bc9a68

public static final String BASE_API_URL = "https://api.mapbox.com";

doesn't include it.

BTW code wise LGTM 👍

[korshaknn]

@Guardiola31337 baseUrlWithLastSlash tests it

.baseUrl("https://mapbox.com/")

[RingerJK]

@Guardiola31337 do you think it's fine to bring okHttp dependency to the models model?

[Guardiola31337]

This is a great question. Yeah, ideally services-directions-models doesn't have dependencies. We're including OkHttp in services-core

mapbox-java/services-core/build.gradle

Line 44 in 7bc9a68

api dependenciesList.retrofit

mapbox-java/services-core/build.gradle

Line 51 in 7bc9a68

api dependenciesList.okhttp3Logging

Can we workaround it somehow?

[korshaknn]

as I said we can't use java.net.URL because it doesn't have handy methods to build URL, we will struggle with / symbols anyway, I haven't found any workaround so use okhttp HttpUrl

[RingerJK]

let's migrate toUrl method to the service module, smth like

class ServiceUtils {
  public static String toUrl(RouteOption routeOptions) { 
    ...
  }
}

and deprecate current fun. This way we don't need okhttp dependency in the current module

[RingerJK]

kotlin extensions would look better 💯

[RingerJK]

I vote to move toUrl to the service module. Models has to be as dumb as possible and assembling URLs based on fields is not what models should make

[korshaknn]

@LukasPaczos @Guardiola31337 vote please and I will continue with the approach

[LukasPaczos]

We need to maintain the current version of RouteOptions::toUrl anyway, we don't want to release a new breaking mapbox-java release, so I would vote for this option. The method does not need to do any encoding, just a simple unencoded URL that HTTP clients can then interpret. We can migrate to string builders if that's easier and cover edge cases with tests.

[korshaknn]

ok, 2-1 wins stringBuilder + tests

[Guardiola31337]

Late to the party but I'd vote StringBuilder + tests too 😅

[korshaknn]

I've fixed // issue and added a test.
cc @LukasPaczos @RingerJK

[RingerJK]

LGTM