RFR 8148316: jshell tool: Configurable output format

[Robert Field]

On 02/26/16 05:20, Jan Lahoda wrote:
> 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

;-)  Oops!

>
>
> -"/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)

I'll clean up the help.

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

Good idea.

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

Oy! Reads like nonsense without some punctuation.  How about a colon?

       |  Error: Not a valid case: foo -- must be one of: when action 
resolve name type result pre post errorpre errorpost

OK, will fix the double |   |

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

Thanks.

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

Moving the format command class to its own top-level class/file is the 
reorganization I was referring to when you told me you were in the 
middle of reviewing, so I deferred.  Will do that now.

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

I set that apart to reduce clashes on merging with the /help <command> 
changeset.  I can merge in now.

Thanks!

Will update and send out a new webrev.

-Robert

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