----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59736/#review176794 -----------------------------------------------------------
1) `¯_(?)_/¯` 2) I think the setting of defaults would belong to the CommandArguments builder. I think it would read nicely for that to be wrapped in a `try` with the error returns in lines 225 and 235 as caught exceptions. 3) I think this overlap could be resolved cleanly with a `LocatorLauncher.Builder.buildFromArgs(argsObject)` method. We already have a `*.parseArguments`, which could be removed/depricated to help cleanly define that line if we want. 4) I like the `(method, args)` as a paradigm, but I don't know how extensive an effort would be needed to standardize this across the codebase. - Patrick Rhomberg On June 1, 2017, 11:10 p.m., Jared Stewart wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59736/ > ----------------------------------------------------------- > > (Updated June 1, 2017, 11:10 p.m.) > > > Review request for geode. > > > Repository: geode > > > Description > ------- > > I'm having trouble finding a way to test the actual product changes for > GEODE-2933 that doesn't feel hacky. I don't intend to commit this review > request in the current state. Rather, I wanted to start a discussion to > validate the general strategy and consider alternative suggestions before I > get too far into the work. > > There is a StartLocatorCommandArguments class introduced that makes easier > passing around all of the arguments to startLocator(). It also allows setting > one or two arguments for tests without needing to put the other 20 nulls in > the correct positions. Thoughts/questions I still have: > > 1) Does the improved testability justify the added boilerplate of > StartLocatorCommandArguments? > 2) Should the setting of "default values" (lines ~221-246 in > LauncherLifecycleCommands) also be moved into this class? > 3) It seems like there some overlap between the intent of > StartLocatorCommandArguments and LocatorLauncher.Builder - not sure where the > boundary between these two should be. > 4) There are more places that `args` could replace telescoped methods. E.g. > `createStartLocatorCommandLine` could take in `(locatorLauncher, args)` > rather than ```(locatorLauncher, > gemfirePropertiesPathname, gemfireSecurityPropertiesPathname, > gemfireProperties, > classpath, includeSystemClasspath, jvmArgsOpts, initialHeap, > maxHeap)``` > > > Diffs > ----- > > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java > 4c668b6813de71aae9e3e46c82a5c231a41c1f6a > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/lifecycle/StartLocatorCommandArguments.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java > 9f68d3a5eaadbe8f1bd95ec8df85ed1f65aa67ce > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/lifecycle/StartLocatorCommandArgumentsTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/59736/diff/1/ > > > Testing > ------- > > > Thanks, > > Jared Stewart > >
