geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kirk Lund <kirk.l...@gmail.com>
Subject Re: Review Request 58589: GEODE-1597: use Spring shell's parser and delete our own parsing code
Date Wed, 26 Apr 2017 21:09:48 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58589/#review173100
-----------------------------------------------------------


Ship it!




Ship It!

- Kirk Lund


On April 26, 2017, 7:48 p.m., Jinmei Liao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58589/
> -----------------------------------------------------------
> 
> (Updated April 26, 2017, 7:48 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-1597: use Spring shell's parser and delete our own parsing code
> 
> * deleted usage of jopsimple and our own parser code
> * reworked help/hint
> 
> The biggest change is in GfshParser, instead of directly implement SpringShell's Parser,
now it's just a simple wrapper around SpringShell's SimpleParser so that we can use spring's
parsing code. The challenge is that SimpleParser expects the user inupt to be in the format
of "command option1 value1 option2 value2", while gfsh expects the format to be like "command
option1=value1 option2=value2", so most of the code in GfshParser is to turn the input into
the SimpleParser input format and then feed it into SimpleParser to get the validation and
autocompletion we needed.
> 
> So far the difference I noticed with this new implementation are:
> 1) option validation is what we gain from directly using SpringShell.
> 2) SpringShell doesn't allow you to specify an option twice, so for multivalued parameters,
our old parser can accept command like "change loglevel --member=member1 --member=member2",
but now the parser will give you an error, and you should only do "change loglevel --member=member1,member2".
The new parser did something speical for --J option though, so for --J, you can specify it
multiple times. 
> 3) For value separator, springShell default's to ",", you can only overwrite it with
option context "splittingRegex", we do not honor the @CliMetaData(valueSepartor=) anymore.
All of our commands uses "," for separators, so this won't break our commands, but if there
is any command out there that would define a different @CliMetaData(valueSepartor=) other
than ",", SpringShell would not know how to parse it.
> 4) To specify empty string as parameter value, before you will need to to (put region=A
key="''" value="''"), spring shell would think the value you are trying to pass in are two
single quotes instead of empty strings, now, you should only do (put region=A key="" value="")
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
44004454ef1646bfeca8a7af3cffb109fd83e7d7 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java a1d03e45dd4d6559bd9a0869c7dd95908d1858ca

>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/GfshHelpCommands.java
1d1b28e568a1e175690fea5cde1a45a318b6db5d 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java
b37feabe6ed61c081e9653c94128f092ad189d10 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java
PRE-CREATION 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java
c346eaf77087102f51952a567ecd7ec036593a13 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java bcf1b415e9ee85822c470ae6888920f0a90159fd

>   geode-core/src/test/java/org/apache/geode/management/internal/cli/CommandManagerJUnitTest.java
503ffb2929758d617e8539e6aefb3f7b545de9b6 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserIntegrationTest.java
f3e3bd8754e18d7a75faf4b75e3ba75b778fc9d7 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserJUnitTest.java
44e99f461b9efc5a439e629751011a6d0edd9213 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CliCommandTestBase.java
165f66482758dadb75ae95d23443973e9de05891 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelpBlockUnitTest.java
PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelperUnitTest.java
PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategyJUnitTest.java
54c7cf7074435b48232b61916627eed69a201f09 
> 
> 
> Diff: https://reviews.apache.org/r/58589/diff/4/
> 
> 
> Testing
> -------
> 
> precheckin successful with the latest changes.
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>


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