gabriel-tincu commented on pull request #979: URL: https://github.com/apache/avro/pull/979#issuecomment-730169687
> This is great, thanks for taking it on! I have a few thoughts I think can improve it: > > 1. Even though this is a port of a Java implementation, I don't think we need the static methods -- those aren't necessary in Python. Instead, consider making them "just functions" in the module. > > 2. The major parts of the implementation would be much better if they had some docstrings explaining them. > > 3. Mutating 'location' in a few places is a little obscure. Ideally I'd prefer to avoid mutation altogether, but it has its uses. If it's unavoidable, then if there's a way to make it visible and easy to trace it will really help debugging later on. Dropped the static methods, addressed most of the PR comments. Added some docstrings lifted from the java codebase (on the more meatier methods). Also added some comments wrt the location issue ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
