tkobayas commented on PR #5827: URL: https://github.com/apache/incubator-kie-drools/pull/5827#issuecomment-2036591991
Hi @yurloc @gitgabrio @mariofusco I think of some approaches for this refactoring and not very sure which one is the best. I'd like to hear your thoughts. The point is to make sure `Descr`'s common properties (e.g. `startCharacter`) are set. The current implementation explicitly calls `DRLVisitorImpl.populateStartEnd()` to populate them. So we'd like to do it implicitly. A) Introduce a constructor `BaseDescr(BasicDescrContext ctx)` where `BasicDescrContext` is a new interface to hold basic information like `startCharacter`. We can specify a super class of generated Context classes by an option `contextSuperClass` (https://github.com/antlr/antlr4/blob/master/doc/options.md#contextsuperclass). So we can pass the context class to the constructor without making `drools-drl-ast` depending on antlr4. The PR is https://github.com/apache/incubator-kie-drools/pull/5826 B) Introduce a helper method like `DescrHelper.init(new PackageDescr(), ctx)`. This doesn't hide a Descr constructor, so is not much different from the current code (= calling `populateStartEnd()`). However, new contributors will understand how to initialize the Descr object and make sure common properties are set. It is this PR. https://github.com/apache/incubator-kie-drools/pull/5827 C) Introduce a factory method like `DescrHelper.createPackageDescr(ctx)` replacing the option B's `DescrHelper.init(new PackageDescr(), ctx)`. It can hide the Descr constructor. Instead, `DescrHelper` would have a bunch of similar factory methods. e.g. ``` pablic static PackageDescr createPackageDescr(DRLParser.CompilationUnitContext ctx) { return populateCommonProperties(new PackageDescr(), ctx); } pablic static RuleDescr createRuleDescr(DRLParser.RuledefContext ctx) { return populateCommonProperties(new RuleDescr(), ctx); } ... ``` I haven't wrote this version yet, but it's easy to write it on top of option B. --- Option A looks smarter. But actually, I had to modify many codes and also I still need `DescrHelper` for some special cases (root AndDescr, PatternBind). Option B and C may not be smart, but I think it's easier to understand/maintain. I'm leaning towards option C. WDYT? -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
