geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jared Stewart <jstew...@pivotal.io>
Subject Re: Review Request 59686: GEODE-2983: correctly handling --J option value that has ", " inside.
Date Wed, 31 May 2017 22:18:36 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message