Mailing-List: contact dev-help@geode.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@geode.apache.org
Content-Type: multipart/alternative;
 boundary="===============8903749838280203576=="
MIME-Version: 1.0
Subject: Re: Review Request 59686: GEODE-2983: correctly handling --J option
 value that has ", " inside.
From: Jared Stewart <jstewart@pivotal.io>
To: Kirk Lund <kirk.lund@gmail.com>, Patrick Rhomberg <prhomberg@pivotal.io>,
 Emily Yeh <eyeh@pivotal.io>, Jared Stewart <jstewart@pivotal.io>,
 Ken Howe <khowe@pivotal.io>
Cc: Jinmei Liao <jiliao@pivotal.io>, geode <dev@geode.apache.org>
Date: Wed, 31 May 2017 22:18:36 -0000
Message-ID: <20170531221836.8717.58340@reviews-vm2.apache.org>
Auto-Submitted: auto-generated
Sender: Jared Stewart <noreply@reviews.apache.org>
References: <20170531174248.8718.75565@reviews-vm2.apache.org>
In-Reply-To: <20170531174248.8718.75565@reviews-vm2.apache.org>
Reply-To: Jared Stewart <jstewart@pivotal.io>
archived-at: Wed, 31 May 2017 22:18:47 -0000

--===============8903749838280203576==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


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


--===============8903749838280203576==--