zcsizmadia commented on pull request #1578: URL: https://github.com/apache/avro/pull/1578#issuecomment-1060636467
Looks ok, however it is always hard to say so when there are no unit tests. It is not @kordos fault, simply we just dont have coverage for avrogen. I have #1571, which test coverage for avrogen, however it must be reviewed, since there is some code shuffling there. Latr today I will for just testing add this feature to my local branch and validate with unit testing, just to be sure this change has no no unknown side effect. So merging this is not blocked by #1571. I am not sure how I feel about the name itself. My first reaction to the name if I would see it for the first time is, that the actual namespace to the classes are flattened as well, not just the directory structure. (Maybe `-flatten-directories`). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
