[GitHub] [avro] epalace edited a comment on pull request #965: AVRO-2937: Add some missing options in SpecificCompilerTool

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

epalace edited a comment on pull request #965:
URL: https://github.com/apache/avro/pull/965#issuecomment-708512156


   Thanks for the comments @RyanSkraba,
   
   >Hello! I noticed that the flag names are different than the ones used in the schema compiler maven goal. Do you think it's worthwhile trying to "unify" them?
   - I unified the two flags `gettersReturnOptional` and `optionalGettersForNullableFieldsOnly` into an optional enum in order to avoid the unfortunate case of setting the former to false while the latter to true. (By the way I'm new to this, what's the point of allowing to generate getters for _non_ nullable fields? is due to backwards compatibility issues?)
   - I also renamed `createOptionalGetters` to `addExtraOptionalGetters` in order to make more explicit that this is adding some extra getters (not modifying the original ones), and also to avoid confusion with the quite similar flag `gettersReturnOptional`
   .
   
   That said, you're right that unifying them would be a good idea. Do you think this new nomenclature makes sense or we should stick to the original one?
   
   >I really like the separation into CompilerOptions for readability! What do you think about moving it into the SpecificCompiler class directly, so it could be reused as "the source of truth" for configuration options and the Java code generation?
   
   I makes perfect sense to do that and I would be happy to do that refactor, but I have a few concerns: 
   - This would be a breaking change in SpecificCompiler API, would that be an issue? Should we maintain compatibility with the old getters and setters and deprecate them?
   - This refactor would be an unrelated change to the subject of this PR, leading to potentially many irrelevant changes. For example, the template `record.vm` would need to change, to access inner fields within the CompierOptions object. Would it make sense to create a separate PR?
   
   Regards.
   


----------------------------------------------------------------
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:
users@infra.apache.org