[GitHub] [avro] zcsizmadia commented on pull request #1515: AVRO-3364 Breakout subclasses for GenericDatumReader

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

zcsizmadia commented on pull request #1515:
URL: https://github.com/apache/avro/pull/1515#issuecomment-1030032462


   There is no API break with this change, afaict, I did not compare every piece of the classes which were moved into a new file, since the change is not really diffable, however it looks ok froma  syntax point of view.
   
   @KyleSchoonover I did read the ticket and it says this is for making the original file smaller. Could you give some more detail on this change? Are you planning to introduce some future feature, which requires this?
   
   Here is my 2 cents on using partial classes:
   
    I use it a lot, especially (I might say only) for generated classes. E.g. I extencd avrogen generated files with functionality. Since avrogen creates a partial class, I can add my own functionality easily by wruting my addition in a different partial class, with the same name.
   
   I see the subclasses in the class, which you created the partial class files for. IMO having those sub/helper classes is not a practice I follow, since it makes the code harder to read, but even the dotnet runtime source code has many cases  of subclasses. However if the sub classes are getting bigger in size, it makes sense to put them in their own partial class file.  This one particular file and the sub classes inside of it did not reach my personal threshold of being unmaintable or unreadable (yet), but of course that is definiteley a personal preference or opinion.
   


-- 
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: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org