search

whosonfirst / whosonfirst-tests

Testing records in Who's On First
0 stars 2 forks source link

property_types: add wof:population as integer type #11

Closed missinglink closed 7 years ago

missinglink commented 7 years ago

hey, I noticed the wof:population properties are being encoded as strings when they should probably be integers.

missinglink commented 7 years ago

example commit: https://github.com/whosonfirst-data/whosonfirst-data/commit/c51d1e75239fb1c9c8d61f7151d180981f7022b6

nvkelso commented 7 years ago

@dphiffer This edit came from Boundary Issues and produced a string property value (should be an int). Are we missing an entry in a schema config somewhere that says this property should only have ints?

stepps00 commented 7 years ago

Updated property_types file in https://github.com/whosonfirst/whosonfirst-tests/commit/6bb1f792a48dc7c7d3f049bdb0883988dc94afbf to include wof:population and wof:population_rank (both as "int" types).

nvkelso commented 7 years ago

@dphiffer after @stepps00 change in https://github.com/whosonfirst/whosonfirst-tests/commit/6bb1f792a48dc7c7d3f049bdb0883988dc94afbf do you need to update anything in Boundary Issues to pickup that schema change?

stepps00 commented 7 years ago

Related:

dphiffer commented 7 years ago

I just added wof:population and wof:population_rank to the schema, which I think should help. (see: https://github.com/whosonfirst/whosonfirst-www-boundaryissues/commit/f5e885eb384986e4d05bbaf4654c787a57514b53)

There is more to this issue, unfortunately, but maybe not especially relevant here. Basically when the edit page loads it includes clues which about datatype is expected, embedded in the HTML. So now when we save the record with an existing wof:population property it should take effect (see: https://github.com/whosonfirst-data/whosonfirst-data/commit/4aaeb12b8457d4ddc15b01267c98d18eb4f5c977).

Unfortunately the schema isn't consulted when a new property gets added to a record, so I still haven't totally addressed this in BI.

stepps00 commented 7 years ago

Closing this PR - the property json was updated in a prior commit to include these changes.

https://github.com/whosonfirst/whosonfirst-tests/blob/master/property_types.json#L47-L48