imply-cheddar commented on code in PR #16798:
URL: https://github.com/apache/druid/pull/16798#discussion_r1707300558
##########
sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java:
##########
@@ -544,18 +545,20 @@ public SqlStatementFactory statementFactory()
* This is an intermediate solution: the ultimate solution is to create
things
* in Guice itself.
*/
- private class TestSetupModule implements DruidModule
Review Comment:
This is an aesthetic nit, but it looks like you are creating the
`CompositeDruidModule` in order to get some reuse for the `getJacksonModules()`
call? By doing it, you are now needing to add things to the constructor with
`super(new BuiltInTypesModule(), new TestSqlModule())` and you are needing to
remember to call `super.configure(binder)`, but there is no call to
`super.getJacksonModules()` because this class isn't adding any Jackson
modules. If it started adding it's own JAckson modules, it would end up doing
an override with a call to super and then adding things to the List.
I'm not sure that this setup with a parent class is actually simpler than
having a field
```
private final List<DruidModule> subModules = Arrays.asList(new
BuiltInTypesModule(), new TestSqlModule());
```
And then in `configure()` adding
```
for (DruidModule module : subModules) {
binder.install(module)
}
```
And then implementing `getJacksonModules()` in the same way that the parent
is doing it.
You could argue that if something else also needs to do this sort of thing,
then it will be repeating these for loops, but honestly, I would tend to think
it's better to repeat the for loops and have the code be explicit about what
it's doing than have to remember to call the super method and have split
between a parent and the child.
--
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]
