Skip to content

Support GeometryCollection geo_shape types and queries #2532

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 1 commit into from
Jan 11, 2017

Conversation

russcam
Copy link
Contributor

@russcam russcam commented Jan 11, 2017

Tidy up xml comments on queries
Fix warnings

Closes #2387

@@ -8,7 +8,7 @@ internal class GeoCoordinateJsonConverter : JsonConverter
public override bool CanConvert(Type objectType) => true;
public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
{
var p = value as GeoCoordinate;
var p = (GeoCoordinate)value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this blow up if value isn't a GeoCoordinate? Not sure if the null check makes sense anymore with the direct cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would. yes 😄

My thinking for converting to a direct cast however is that the converter is internal so we have control over which types it is applied to, so we should only be applying it to GeoCoordinate. If it's applied to another type other than GeoCoordinate, I think it would be better to have the invalid cast exception (to fix) rather than swallowing. In addition, the direct cast will slightly faster than as, which is effectively an is check and then cast.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ but I don't think the null check makes sense anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The null check should still be there because it could be passed a null GeoCoordinate still

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, ok 👍

@gmarz
Copy link
Contributor

gmarz commented Jan 11, 2017

Minor comment, but otherwise LGTM.

@russcam russcam force-pushed the feature/5.x-geometrycollection branch from 30b7fd8 to 42dcf97 Compare January 11, 2017 22:38
Tidy up xml comments on queries
Fix warnings

Closes #2387
@russcam russcam force-pushed the feature/5.x-geometrycollection branch from 42dcf97 to fe41648 Compare January 11, 2017 22:42
@russcam russcam merged commit fe41648 into 5.x Jan 11, 2017
@Mpdreamz Mpdreamz deleted the feature/5.x-geometrycollection branch January 23, 2017 21:14
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.

2 participants