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

Fixture 013 #19

Merged
merged 5 commits into from
Sep 15, 2017
Merged

Fixture 013 #19

merged 5 commits into from
Sep 15, 2017

Conversation

springmeyer
Copy link
Contributor

@springmeyer springmeyer commented Sep 6, 2017

Adding a new fixture for

Key-mistyped_uint32.mvt: Has a key property incorrectly encoded as a type std::uint32_t. | n/a

From #15 into the branch from #18 to get a feel for things. They feel good overall!

Two concerns that came up:

  • 🍍 For tiles generated with a custom proto, they won't decode quite right with the scripts/dump command which currently hardcodes the 2.1 proto.
  • 🍒 I ran npm run docs and saw that updates both the FIXTURES.md and API.md. I would have expected npm run build to update the FIXTURES.md and npm run docs to just update API.md

As far as 🍍 this is somewhat problematic. We should probably not hardcode like I did in #17 because then it is hard to use the script to determine if your resulting .mvt fixtures is correctly incorrect. Ideas to solve this are:

  • dump the full .proto string to a file in the fixtures/ directories and then rig the scripts/dump to ingest a fixture folder path and to autmatically find the right .proto on the system.
  • have the scripts/dump parse the file without using the proto by using the --decode_raw trick I mention at Add script to decode raw tiles #17

This also raises the fairly meta question in my mind of: how do we test the fixtures? Should we have a way to assert, as part of the build process, that they contain the exactly wrong data we assume?

cc @mapsam

@springmeyer springmeyer mentioned this pull request Sep 6, 2017
@mapsam mapsam changed the base branch from edit-protos to core-tech-fixtures September 15, 2017 16:35
@mapsam mapsam changed the title Fixture 010 Fixture 013 Sep 15, 2017
@mapsam
Copy link
Contributor

mapsam commented Sep 15, 2017

Thank you @springmeyer - looks like 010 was already created in #27 so I've renamed this to 013 and resynced with core-tech-fixtures.

@mapsam
Copy link
Contributor

mapsam commented Sep 15, 2017

For tiles generated with a custom proto, they won't decode quite right with the scripts/dump command which currently hardcodes the 2.1 proto.

Per discussion we are happy with the --raw_decode functionality for now using protoc.

docs to build with npm run build

Also per discussion, sounds like keeping docs separate to avoid merge conflicts is 👍

@mapsam mapsam merged commit 693b83f into core-tech-fixtures Sep 15, 2017
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.

2 participants