martin-g commented on PR #1833: URL: https://github.com/apache/avro/pull/1833#issuecomment-1229887817
> My biggest concern is that user are not able inject own custom logic without changing library. If you add more interfaces and use interface instead of concert classes as dependency, it allows users who want benefit from dependency injection use it. As far as I understand the code the encoder/decoder code does not use `static` methods/fields and it is easily extendable/overrideable. `Symbol` class has `static`s indeed! Although they could be explained as an implementation detail of the JsonDecoder I agree that it would be better if their usage is reduced as much as possible. For example adding unit tests for each new class (like JsonDecoderTests.cs, JsonEncoderTests.cs, Symbol.cs) would show us what is well designed and extendable, and what is problematic due to the usage of `static`s. At the moment we have only JsonCodecTests.cs which is rather higher level / functional test. To summarize my review: I think the PR needs more unit tests, especially ones testing invalid input: wrong JSON, wrong values for the expected schema, etc. -- 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]
