Skip to content

Added a null check to GeoLocation and updated ToString() #1814

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.

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

Merged
merged 2 commits into from
Feb 15, 2016
Merged

Added a null check to GeoLocation and updated ToString() #1814

merged 2 commits into from
Feb 15, 2016

Conversation

niemyjski
Copy link
Contributor

The ToString() must be in a specific format for lat long of which we know...

Also there is only so much precision you can specify (http://www.geomidpoint.com/latlon.html)

You wouldn't want a number converted to 1.3e6 or 1.12345678901234545666666556 when 8 points of precision gets you to 1/16th of an inch

Mpdreamz added a commit that referenced this pull request Feb 15, 2016
Added a null check to GeoLocation and updated ToString()
@Mpdreamz Mpdreamz merged commit 8f81232 into elastic:master Feb 15, 2016
@Mpdreamz
Copy link
Member

This LGTM 👍 thanks @niemyjski

I'd like to open the discussion though that our implicit conversions should not throw but return null as per c# guidelines /cc @gmarz @russcam

@niemyjski
Copy link
Contributor Author

hmm, yeah that's a good point.

@niemyjski
Copy link
Contributor Author

https://msdn.microsoft.com/en-us/library/ms229033.aspx I wasn't aware of that rule..

@russcam
Copy link
Contributor

russcam commented Feb 16, 2016

Agreed on the implicit conversions returning null instead of throwing; if there is a need for a conversion to throw then it should ideally be explicit but then that runs contrary to the value of the implicit conversion 😄

@gmarz
Copy link
Contributor

gmarz commented Feb 16, 2016

Agreed on the implicit conversions returning null instead of throwing

+1 good call @Mpdreamz

@niemyjski
Copy link
Contributor Author

Do you guys want me to fix this.. looks like there is still an issue with this format: exceptionless/Exceptionless.Net#80

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants