Use lbl:bbox property when present #122

Merged
merged 2 commits into from Aug 9, 2016

Conversation

Projects
None yet
3 participants
@orangejulius
Member

orangejulius commented Aug 5, 2016

After the discussion in pelias/pelias#370,
Who's on First implemented the lbl:bbox field, which is a hand-made
bounding box for areas where the mathematically derived bounding box
doesn't match up with what people generally want.

At least two of these were implemented in
whosonfirst-data/whosonfirst-data#361, so it's
time to start using them!

Connects pelias/pelias#370
Connects pelias/pelias#397

Use lbl:bbox property when present
After the discussion in pelias/pelias#370,
Who's on First implemented the `lbl:bbox` field, which is a hand-made
bounding box for areas where the mathematically derived bounding box
doesn't match up with what people generally want.

At least two of these were implemented in
whosonfirst-data/whosonfirst-data#361, so it's
time to start using them!

Connects pelias/pelias#370
Connects pelias/pelias#397

@orangejulius orangejulius self-assigned this Aug 5, 2016

src/components/extractFields.js
@@ -38,6 +38,14 @@ function getLon(properties) {
}
}
+function getBoundingBox(properties) {
+ if (properties['lbl:bbox']) {

This comment has been minimized.

@dianashk

dianashk Aug 5, 2016

Contributor

would be better to use hasOwnProperty or isSet here

@dianashk

dianashk Aug 5, 2016

Contributor

would be better to use hasOwnProperty or isSet here

This comment has been minimized.

@orangejulius

orangejulius Aug 8, 2016

Member

Hm I think you're right, as well as for the other helper functions. For lat/lon for example, I bet we don't handle null island very well.

@orangejulius

orangejulius Aug 8, 2016

Member

Hm I think you're right, as well as for the other helper functions. For lat/lon for example, I bet we don't handle null island very well.

This comment has been minimized.

@orangejulius

orangejulius Aug 8, 2016

Member

Upon further reflection, I now disagree (although not very strongly). The only differences between if (object['prop')) and if (obect.hasOwnProperty('prop')) are how it handles the number zero and empty strings. For bounding boxes, neither of those are valid values, so falling back to the standard bbox is fine.

For other fields it's mostly the same way. With population we explicitly want to ignore zero values and fall back, and while I mentioned the null island case (where we actually do incorrectly fall back to the geom lat/lon because the lbl lat/lon is 0, 0), it feels like the simplicity of the current syntax is worth the slight mishandling of that one case.

Am I missing an edge case that we are protected against with hasOwnProperty? I think I covered all of them

@orangejulius

orangejulius Aug 8, 2016

Member

Upon further reflection, I now disagree (although not very strongly). The only differences between if (object['prop')) and if (obect.hasOwnProperty('prop')) are how it handles the number zero and empty strings. For bounding boxes, neither of those are valid values, so falling back to the standard bbox is fine.

For other fields it's mostly the same way. With population we explicitly want to ignore zero values and fall back, and while I mentioned the null island case (where we actually do incorrectly fall back to the geom lat/lon because the lbl lat/lon is 0, 0), it feels like the simplicity of the current syntax is worth the slight mishandling of that one case.

Am I missing an edge case that we are protected against with hasOwnProperty? I think I covered all of them

This comment has been minimized.

@dianashk

dianashk Aug 9, 2016

Contributor

I think you're right about the various conditions and the code will work fine for this scenario. However, it's not a great convention and doesn't make the true intent of this statement clear. I recall discussing this with the team a long time ago and coming to an agreement to use hasOwnProperty or isSet checks for these cases. It's not a big deal, but I think it makes the code a bit easier to understand.

@dianashk

dianashk Aug 9, 2016

Contributor

I think you're right about the various conditions and the code will work fine for this scenario. However, it's not a great convention and doesn't make the true intent of this statement clear. I recall discussing this with the team a long time ago and coming to an agreement to use hasOwnProperty or isSet checks for these cases. It's not a big deal, but I think it makes the code a bit easier to understand.

This comment has been minimized.

@orangejulius

orangejulius Aug 9, 2016

Member

You're right, i remember that conversation as well. Code updated!

@orangejulius

orangejulius Aug 9, 2016

Member

You're right, i remember that conversation as well. Code updated!

Check for existence of lbl:bbox property
This is clearer than checking for truthiness
@dianashk

This comment has been minimized.

Show comment
Hide comment
@dianashk

dianashk Aug 9, 2016

Contributor

LGTM 🌴 🌴 🌴

Contributor

dianashk commented Aug 9, 2016

LGTM 🌴 🌴 🌴

@trescube

This comment has been minimized.

Show comment
Hide comment
@trescube

trescube Aug 9, 2016

Contributor

:shipit:

Contributor

trescube commented Aug 9, 2016

:shipit:

@orangejulius orangejulius merged commit 0f4e787 into master Aug 9, 2016

3 checks passed

approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@orangejulius orangejulius removed the in review label Aug 9, 2016

@orangejulius orangejulius deleted the lbl_bbox branch Aug 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment