Use lbl:bbox property when present #122
Conversation
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
| @@ -38,6 +38,14 @@ function getLon(properties) { | |||
| } | |||
| } | |||
|
|
|||
| function getBoundingBox(properties) { | |||
| if (properties['lbl:bbox']) { | |||
There was a problem hiding this comment.
would be better to use hasOwnProperty or isSet here
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You're right, i remember that conversation as well. Code updated!
This is clearer than checking for truthiness
|
LGTM 🌴 🌴 🌴 |
|
|
After the discussion in pelias/pelias#370,
Who's on First implemented the
lbl:bboxfield, which is a hand-madebounding 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