add SerializeToLonLat function and switch web calls to use that

[brnkhy]

change ToString back to its default behaviour

[david-rhodes]

Well, this is awkward. I found out why I had to override ToString with longitude as the first component (x). If you check Drive.unity, you will get a NullReferenceException caused by the directions request. This is because deep down in the url formatting for directions waypoints is this:

protected static string GetUrlQueryFromArray<U>(U[] items, string separator = ",")
{
    return string.Join(separator, items.Select(item => item.ToString()).ToArray());
}

This is using ToString() rather than SerializeToLonLat(). It was hard to find this because of the generic usage here.

Not sure if this is an easy fix or even worth it.

[david-rhodes]

I'd also like to note that if feels "dangerous" to expect ToString to format for URL requests specifically, as ToString is also often used for lots of other things.

[wilhelmberg]

Reminder: please don't forget about unit tests, either run them locally or check on AppVeyor. P.S.: check the branch build as the PR build is expected to always fail on AppVeyor. This is a security feature as encrypted environment variables are not decrypted for PRs.

eg.: https://ci.appveyor.com/project/Mapbox/mapbox-sdk-unity-core/build/1.0.332/job/8djpnr40f6xbnb1h/tests

---

This is using ToString() rather than SerializeToLonLat(). It was hard to find this because of the generic usage here.

I'd also like to note that if feels "dangerous" to expect ToString to format for URL requests specifically, as ToString is also often used for lots of other things.

What about about dedicated methods for each use case?

• ToUrlString()
• SerializeToLonLat()
• ...

[david-rhodes]

What about about dedicated methods for each use case?

This is probably fine, but will require sweeping changes, with many classes affected, right?

[wilhelmberg]

@brnkhy do you still want to get this in before we freeze or is this PR stale?

[brnkhy]

@BergWerkGIS I think we should, I feel like overriding ToString like that is fundamentally wrong so if you guys agree, let's not ship like that.

[wilhelmberg]

@brnkhy I, too, like methods to be as verbose as possible (and needed).

So how about SerializeToLonLat() and for the sake of completeness also SerializeToLatLon()?

And let ToString() return something like

string.Format(NumberFormatInfo.InvariantInfo, "X:{0:F5} Y:{1:F5}", this.x, this.y);

which would have the advantage to show up nicely in the debugger.

Also: please evaluate the consequence of this change in mapbox-unity-sdk (see above) and queue up a PR there that takes care of needed changes.

[brnkhy]

@BergWerkGIS SerializeToLonLat is already there we can add SerializeToLatLon as well, it's not used anywhere but maybe just for completeness as you said. I think I would prefer to keep ToString in original format even though adding x/y labels like that won't change behaviour. ToUrlString is a closed term as well. please evaluate the consequence of this change you mean the tests? I'll check them 👍

[wilhelmberg]

I think I would prefer to keep ToString in original format even though adding x/y labels like that won't change behaviour. ToUrlString is a closed term as well. please evaluate the consequence of this change you mean the tests? I'll check them 👍

Yes, tests in this repo (see above).

But as well the consequences of this PR in mapbox-unity-sdk as mentioned by @david-rhodes, eg the Drive example (see above). I think we are going to need a PR in mapbox-unity-sdk as well, no?

[david-rhodes]

@BergWerkGIS @brnkhy As mentioned here: https://github.com/mapbox/mapbox-sdk-cs/pull/71#issuecomment-298981632, I think this refactor is more work than we think. GetUrlQueryFromArray is used by multiple objects--not just directions.