----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59686/#review176535 -----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java Lines 48 (patched) <https://reviews.apache.org/r/59686/#comment249916> I worry that a user may at some point specify a `--J` option that includes `,,`. I think our code could be more robust by using a delimeter that a user can't type on a standard keyboard. The ASCII "Unit separator" character (decimal code 31, hex 0x1f) seems like a natural candidate. That would make this look like: ``` private static final char ASCII_UNIT_SEPARATOR = '\u001F'; public static final String JARGUMENT_SPLIREGX = "" + ASCII_UNIT_SEPARATOR; ``` I also think that `J_ARGUMENT_DELIMITER` might be a more informative name. geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java Lines 233 (patched) <https://reviews.apache.org/r/59686/#comment249921> What do you think about adding a field to `GfshParser` that can be referenced by all of these `optionContexts`? Basically: ``` class GfshParser { ... J_ARGUMENT_OPTION_CONTEXT = "splittingRegex=" + JARGUMENT_SPLIREGX; } ``` ``` @CliOption(key = CliStrings.START_LOCATOR__J, optionContext = GfshParser.J_ARGUMENT_OPTION_CONTEXT, help = CliStrings.START_LOCATOR__J__HELP) final String[] jvmArgsOpts, ``` - Jared Stewart On May 31, 2017, 5:42 p.m., Jinmei Liao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59686/ > ----------------------------------------------------------- > > (Updated May 31, 2017, 5:42 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > ------- > > GEODE-2983: correctly handling --J option value that has "," inside. > > > Diffs > ----- > > > geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java > 288ea054ae1230c480d141c0159d6ccf9c299a7d > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java > 74acfd6e03613ac4d0c62fcdd4ea859d1c74d2f2 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserParsingTest.java > 4467792f60a2a3bf7cc53cf35e997e7462882539 > > > Diff: https://reviews.apache.org/r/59686/diff/1/ > > > Testing > ------- > > precheckin running > > > Thanks, > > Jinmei Liao > >
