RFR 8148316: jshell tool: Configurable output format

[Jan Lahoda]

Comments on the code:
-the help could be more descriptive - describing the relationship 
between format and field, etc. Also:
-> /help /set
[snip]
/set

See /set

-> /set
|  The /set command requires arguments. See: /help /set


-"/set format" prints a help, but "/set field" crashes jshell for me:
-> /set field
Exception in thread "main" java.lang.NullPointerException
         at 
jdk.internal.jshell.tool.JShellTool$1FormatSetter.parseFormatSelector(JShellTool.java:1782)
         at 
jdk.internal.jshell.tool.JShellTool$1FormatSetter.setField(JShellTool.java:1592)
         at jdk.internal.jshell.tool.JShellTool.cmdSet(JShellTool.java:1836)
         at 
jdk.internal.jshell.tool.JShellTool.lambda$new$43(JShellTool.java:1435)
         at 
jdk.internal.jshell.tool.JShellTool.processCommand(JShellTool.java:1021)
         at jdk.internal.jshell.tool.JShellTool.run(JShellTool.java:963)
         at jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:760)
         at jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:733)
         at jdk.internal.jshell.tool.JShellTool.main(JShellTool.java:723)

-generally, I'd suggest to also include tests for invalid input

-the output for "/set field foo foo" is:
|  |  Error: Not a valid case: foo -- must be one of when action resolve 
name type result pre post errorpre errorpost

Note the doubled '|', and also I'd suggest to wrap 'when action resolve 
name type result pre post errorpre errorpost' with quotes to make it 
more obvious that is a list of keywords.

-String.toLowerCase() may produce surprising result on some locales, I'd 
suggest to use toLowerCase(Locale.US) in this patch

-the JShellTool.cmdSet method has a very (very) long local class. It 
seems that a class is may be not necessary at all, and the parsing could 
be done using ordinary methods. So I'd suggest to avoid this local 
class, replacing it with methods in the enclosing class. Or, maybe, this 
configurable format is big enough to have its own class?

-nit: the /set command registration has its own initializer block, I 
assume that could be merged into the main command registration block?

Jan

On 20.2.2016 02:23, Robert Field wrote:
> Other changes have gone back since I put this out for review.  This is
> just a refresh sync'ed to the current dev repo.  And with fixed
> copyrights -- thanks, Shinya.
>
> Bugs (the second (unrelated) bug was discovered and fixed in the process
> of testing):
>      https://bugs.openjdk.java.net/browse/JDK-8148316
>      https://bugs.openjdk.java.net/browse/JDK-8149524
>
> Webrev:
>      http://cr.openjdk.java.net/~rfield/8148316v8.webrev/
>
> An example output configuration (Open with /open):
>      http://cr.openjdk.java.net/~rfield/formats/arrowResultNlMeta.jsh
>
> Thanks,
> Robert
>