search

JustinCanton / Geo.NET

A lightweight method for communicating with the multiple online geocoding APIs.
MIT License
13 stars 8 forks source link

Fix string formating culture #51

Closed vanenshi closed 1 year ago

vanenshi commented 1 year ago

Fix the culture problem, affecting the coordination toString function in non-English cultures

JustinCanton commented 1 year ago

Would you be able to tell me the reasoning behind this commit? What were you seeing vs what were you expecting?

vanenshi commented 1 year ago

Hi @JustinCanton We have an API that uses your library for requesting Mapbox reverse geocoding. We are supporting multiple cultures in our API, using Microsoft Culture when the culture is set to English everything work as expected. but when a user request with a different culture (in our case, Turkish) the coordination toString function does not work as expected.

Turkish => 29,2323234545 [Mapbox doesn't understand this] English => 29.2323234545

So I forked your library and fixed the problem like this PR, this time it works normally. but I am not a dotnet expert, you might have a better solution for this. I am ready to contribute.

JustinCanton commented 1 year ago

Thank you for the explanation @vanenshi . I am fine with the change. But as part of the code, I would like to have some unit testing to make sure everything works as expected.

Please add some test cases or theories for this scenario. You can use https://bartwullems.blogspot.com/2022/03/xunit-change-culture-during-your-test.html for information on how to change the culture during a test if you want.

Also, does this impact anything else within the MapBox code? Or is just the Coordinate class impacted?

vanenshi commented 1 year ago

I'll take care of test And I'll check for other edge cases too As far as I know, other parts were not affected.

Just a small question, should I apply this code to other models, or just the map-box? I don't have any token for the other libraries though.

JustinCanton commented 1 year ago

@vanenshi , this will need to be applied to other models for other services. I will create a task to handle that though. It doesn't need to be done in this PR.

vanenshi commented 1 year ago

Awesome Then I'll take care of the mapbox

JustinCanton commented 1 year ago

https://github.com/JustinCanton/Geo.NET/issues/52 has been created to fix the rest of this issue.

JustinCanton commented 1 year ago

@vanenshi, I have decided to release a hotfix for this issue across all nugets. So I will be adding the code fixes into master. I will add you onto the PR for that, but this one will be closed.

JustinCanton commented 1 year ago

@vanenshi, https://github.com/JustinCanton/Geo.NET/pull/53 has been created. I will close this PR.