[GitHub] rob05c commented on issue #3296: Change TO cdns/dnsseckeys to abstract versioning

[GitBox <gi...@apache.org>]

rob05c commented on issue #3296: Change TO cdns/dnsseckeys to abstract versioning
URL: https://github.com/apache/trafficcontrol/pull/3296#issuecomment-460417177
 
 
   Hm. This prevents new fields from ever being null, and not omitted. If a new field is null, it will be omitted from the JSON object returned (because this scheme requires all new fields to be `omitempty`). We can define new fields to behave that way, so it's ok from a versioning standpoint. But are we ok with that limitation? Are you sure we won't ever encounter a scenario in the future, where we need to return `"newField": null`?
   
   I'm also not a fan of the `func(interface{}) interface{}`. Empty interfaces throw away all type safety. At the least, this would really need to be `func(interface{}) (interface{}, error)` to avoid panicing. But even then, it's a runtime error, instead of a compile error like it currently is. Sometimes `interface{}` is inevitable, but it'd be really nice to keep these kind of errors at compile time, if we can.
   
   It'd also be ideal if we could avoid Reflection, which is inevitable with struct tags. I'm less worried about the performance, than the readability. If someone who didn't write it comes along in the future, how difficult will it be to climb the learning curve, for the reflecting code that's parsing the struct tags? 
   
   I'm not outright opposed. But these are the limitations I'm seeing, which would be really nice if we could figure out how to avoid.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services