allow for .proto editing #18
Conversation
There was a problem hiding this comment.
This looks excellent @mapsam. I made some minor comments inline. The only general question I have is whether you considered making the json property a function? If it were a function, and took the schema as an arg, then you could keep the ability to use its enums.
index.js
Outdated
| mvt.Tile.write(json, pbf); | ||
| return new Buffer(pbf.finish()); | ||
| }; | ||
| const eight = require('./src/008.js'); |
There was a problem hiding this comment.
Is this maybe left behind from testing, or does it do something?
| } | ||
|
|
||
| final.id = id; | ||
| final.description = fixture.description; | ||
| final.specification_reference = fixture.specification_reference; | ||
| final.json = fixture.json; | ||
| final.buffer = generateBuffer(fixture.json); | ||
| final.proto = fixture.proto; | ||
| final.buffer = generateBuffer(fixture.json, fixture.proto); |
There was a problem hiding this comment.
I noticed you removed manipulate from the fixtures, but left it here. I like that: we may still want to hook into it for certain fixtures where adding bad data is not easy via just an alternative scheme.
There was a problem hiding this comment.
heh, this was a mistake, but yeah might as well keep it! I'll make sure the starter template includes it with a note.
lib/generateBuffer.js
Outdated
| */ | ||
| module.exports = function(json, proto) { | ||
| let proto_schema; | ||
| if (!proto) proto = '2.1'; |
There was a problem hiding this comment.
How about throwing if no proto is provided? That feels more robust to me since it would ensure each fixture self-documents which spec version it applies do.
scripts/new.js
Outdated
| { | ||
| id: 1, | ||
| tags: [], | ||
| type: schema.Tile.GeomType.POINT.value, |
There was a problem hiding this comment.
This will need updated to be 1 given the current refactor, right?
lib/generateBuffer.js
Outdated
| const Pbf = require('pbf'); | ||
| const Compile = require('pbf/compile'); | ||
|
|
||
| const MVT_SPEC_VERSIONS = ['1.0.0', '1.0.1', '2.0', '2.1']; |
There was a problem hiding this comment.
This should ideally be dynamic. Can you think of a way to read these dynamically to avoid hardcoding?
I removed the function for simplicity, since using the ENUMs provided by pbf felt more complicated than referring to the actual specification (which seems like a good habit to build). Happy to add it back if you think it's important! |
|
@springmeyer I've made changes based on your edits - thank you for the review! Lemme know how things look. |
|
changes look great @mapsam.
Sounds good. |
Implementing idea from @springmeyer in #16 where instead of a
manipulatefunction, we generate a fixture with a manipulated .proto file which allows us to be more confident in value types for each fixture. This does a few things:manipulatefield from the source fixturesprotofields to the source fixtures. This can be a string representing the version, such as2.1or a string representing a full .proto file.protofield less verbose, you can use theutil.replace()method to replace a portion of a currently existing spec .proto file. For example, here's how update a field type.schemaobject in the function. Instead we can use literal values for enumsOne thing to be aware of is our usage of the
mvt.Tile.write()method here. This assumes your custom protocol buffer has aTilemessage at the root of the proto spec. I'm okay with this for now, but you can see in the create.test.js file that the custom .proto file still needs this one reference. Perhaps we want to update generateBuffer to take this into account?Up for review @springmeyer!