mapbox / gazetteer

Place bookmarks for maps
BSD 2-Clause "Simplified" License
10 stars 8 forks source link

Gazetteer 3.0 #19

Closed dasulit closed 6 years ago

dasulit commented 7 years ago

UPDATE: We significantly reworked this version of gazetteer:


Previous summary

Closes https://github.com/mapbox/gazetteer/issues/17 by introducing a new structure for feature properties that closely leans on the Mapbox Streets source!

Closes https://github.com/mapbox/gazetteer/issues/18 by renaming gazetteer.json to mapbox-streets-gazetteer.geojson, with the intent that curated gazetteers can be made for different tilesets. No longer relevant.

Old checklist:

cc @nickidlugash

dasulit commented 7 years ago

Looks like @nickidlugash removed the empty data_layer_field objects where data_layer: building and there are no meaningful data_layer_fields— and @lukasmartinelli added them back to make tests pass 😀 were you intending that the tests should be updated to make data_layer_fields optional, Nicki?

nickidlugash commented 7 years ago

were you intending that the tests should be updated to make data_layer_fields optional, Nicki?

@dasulit I have no preference, I was just looking back at this comment yesterday and thought that your preference was for this field not to exist if it's empty.

nickidlugash commented 6 years ago

Repurposing this PR for the revamped Gazetteer 3.0!

@dasulit @aparlato pls review/comment/edit the spec proposal. I don't know the conventions for writing a spec (e.g. how to indicate optional fields) – please update the formatting if you see anything that could be written more clearly.

Can/should we allow numbers and booleans for tag and data field values?

dasulit commented 6 years ago

Thanks for reviving @nickidlugash!

Can/should we allow numbers and booleans for tag and data field values?

Yep, I think we can and should 👍

aparlato commented 6 years ago

Can/should we allow numbers and booleans for tag and data field values?

Agreed.

@nickidlugash spec proposal looks good to me!

nickidlugash commented 6 years ago

@dasulit @aparlato thanks for the review! Updated based on your comments.

nickidlugash commented 6 years ago

@aparlato I updated the top-level comment with a new summary of gazetteer-3. Please review! 🙏

nickidlugash commented 6 years ago

@dasulit To update tests, looks like we need to update:

  1. the location of the gazetteer from a single file to all files in the mapbox-streets directory
  2. the keys that are being tested (add tag, remove highlights from the "required" test, since we've now made it optional).

I'm happy to make these updates, but i'm not sure what the best way is to do 1.

dasulit commented 6 years ago

@nickidlugash @aparlato I just pushed a commit that fixes the failing tests. Would love for you two to take a quick look at these changes before 🚢

dasulit commented 6 years ago

@aparlato :ducttape: Thanks for your guidance! I incorporated your changes, save for your comment on the lint function, which I responded to inline. Let me know if this all looks good to you.

aparlato commented 6 years ago

@dasulit nice! one last little thing:

We need to move "@mapbox/geojsonhint": "^2.1.0" from devDependencies to dependencies so that it gets installed with the package (I learned this the hard way on highway shield icons module). devDependencies don't get installed with a package and only exist for local development to install while working locally so that they don't bloat apps with stuff they don't need in the wild like js formatting.

dasulit commented 6 years ago

Missed that in the shuffle, should be good now @aparlato.

aparlato commented 6 years ago

@dasulit awesome! 🚢