Github user itkach commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-98543767
> nice idea, but the introduction of new structs within
test/ThriftTest.thrift breaks many other language tests, do you really need new
structs? e.g. BonkMap is available as MapType
`MapType` is a typedef for `map<string,Bonk>`, `BonkMap` is a struct
containing member of type `map<string,Bonk>` - not the same thing.
The unit tests cover several basic cases:
- struct member is a struct (`Xtruct2` fits)
- struct member is a container that contains a struct
* list (`ListBonks` fits)
* set (no suitable existing type, `BonkSet` added)
* map (no suitable existing type, `BonkMap` added)
- struct member is a container that contains container (no suitable
existing type, `Bonkers` added - `CrazyNesting` doesn't fit because it has a
map with a struct as a key which can't work in Javascript and no values that
are structs)
It does appear that new structs are necessary. I understand that adding new
service methods would break other tests, but adding struct definitions
shouldn't break anything, it's really surprising (and wrong)
if it does. If that is indeed the case I can move new struct definitions to
a separate .thrift file to be used just in the deep constructor unit test.
> There is also a inconsistent naming: ListBonks, BonkSet, BonkMap
I can rename `BonkSet` -> `SetBonks`, `BonkMap` -> `MapBonks` to match
`ListBonks` which was there before, although if I need to move structs for the
deep constructor unit test to a separate file anyway I'd probably use different
names altogether. Let me know.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
