Skip to content
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

rename 'polygon' field to 'shape' to match pelias/schema #134

Merged
merged 1 commit into from
Sep 10, 2020

Conversation

missinglink
Copy link
Member

@missinglink missinglink commented Sep 7, 2020

The setPolygon() and getPolygon() methods are replaced by setShape() & getShape().

This ensures the field and method names are consistent with Pelias Schema.

As discussed in pelias/schema#466 the field name should be shape, not polygon.

It's astounding that it's gone 3 years without anyone noticing that it didn't actually work, most likely because it's not very often used.

@missinglink missinglink changed the title rename 'polygon' field as 'shape' to match pelias/schema rename 'polygon' field to 'shape' to match pelias/schema Sep 7, 2020
BREAKING CHANGE: the setPolygon() and getPolygon() methods are replaced by setShape() & getShape().

This ensures the field and method names are consistent with Pelias
Schema.
@orangejulius orangejulius force-pushed the rename_polygon_field_shape branch from e336566 to 27b21bd Compare September 8, 2020 12:05
@orangejulius
Copy link
Member

Yeah, to be clear, outside of little bits of testing out functionality for importing geometries (as part of pelias/whosonfirst#19, pelias/api#1121, etc), this method has never been used.

In the past, we've had a lot of problems with importing geometries into Elasticsearch (poor performance, rejection of large geometries like that of New Zealand by Elasticsearch), but at least theoretically we should allow it. Either that or we should delete the shape field from the schema all together and tell people to use the Spatial service.

@missinglink
Copy link
Member Author

Agh ok, so I'm also not against removing the field instead.

Copy link
Member

@Joxit Joxit left a comment

Choose a reason for hiding this comment

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

I am OK with the removing or renaming of the field. 👍

Both solutions will create a breaking change...

@orangejulius
Copy link
Member

Can't believe we had this bug for 3+ years :)

I'm skeptical Elasticsearch geo_shape fields will ever be performant enough to use, but lets merge this for now and try it out. If not we'll remove the field and go 100% all in on the Spatial service :)

@orangejulius orangejulius merged commit 1a5ae33 into master Sep 10, 2020
@orangejulius orangejulius deleted the rename_polygon_field_shape branch September 10, 2020 15:20
orangejulius added a commit to pelias/whosonfirst that referenced this pull request Sep 10, 2020
This fixes an incorrect model function that was expecting a property
called `polygon` in the Elasticsearch schema. The correct property is
`shape`.

See pelias/model#134
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.

3 participants