Closed andrewcox closed 11 years ago
The negative ids are a facebook thing, the open source generator does not do it. But we could put that under a switch.
minor comments, looks good otherwise.
For the negative ids, I checked and ASF 0.7.0 and 0.9.0 generate these too (so I assume its been like this for a while). Still I find it too wonky to support unless someone absolutely needs it. Explicit field IDs are much easier to use for maintaining backward compat as the .thrift evolves
Addressed feedback, merged, and pushed.
This fixes two things:
If field IDs were missing from the thrift file, the generator was filling them in, but doing it wrong. Generator was supposed to be looking for null for field IDs to indicate missing ids, but the parser was never giving it null because the type was "long" instead of "Long". After this fix, the generator will correctly complain if there are missing field / param identifiers. Another option would be to try to synthesize them "correctly" the way the thrift compiler does, but it does it by synthesizing negative ids and warns that it may not be compatible, so I don't think its worth carrying this over... Seems better to just throw an error and require the .thrift to be explicit.
The other fix is to correctly synthesize integer enumeration values. In this case, there are a number of fbcode .thrift files that take advantage of this (and are being generated incorrectly as of now), and the fbcode & apache thrift compilers don't complain about this and have consistent behavior (count up from last explicit enum value, or from 0 if there hasn't been an explicit value yet), so I made the swift generator do the same thing.