Kulla: JShell API ready for round two review

[A. Sundararajan]

Comments below..

On Wednesday 29 July 2015 06:15 AM, Robert Field wrote:
>> On Jul 27, 2015, at 9:46 PM, A. Sundararajan <sundararajan.athijegannathan at oracle.com> wrote:
>>
>> Looks good.  I was able to write simple nashorn scripts exercising this API using this doc.
> Cool!
>
>> Mostly minor comments..
>>
>> * InternalDebugControl class many public static final constants with no documentation explaining what these are.
>> And the names are not self-explanatory either.
>>
>> http://cr.openjdk.java.net/~rfield/doc/jdk/jshell/InternalDebugControl.html <http://cr.openjdk.java.net/~rfield/doc/jdk/jshell/InternalDebugControl.html>
> I’d prefer completely hide this class, but not sure how to do that.
Hmm.. Non public class or non-exported class?

>
>> * Many pages have <cite> Java language specification </cite>. i.e., HTML tags are visible in documentation pages rather formatted as citations. For example:
>>
>> http://cr.openjdk.java.net/~rfield/doc/jdk/jshell/MethodDeclSnippet.html <http://cr.openjdk.java.net/~rfield/doc/jdk/jshell/MethodDeclSnippet.html>
> So, the <cite> should be removed?

HTML tag is visible "as is" in the output. Citation is formatted as 
"citation". Not sure what is missing in doc comments.

>> * In JShell class, a method may be added (variableValues() ?) that returns a Map<VariableDeclSnippet, String> for all variables. Perhaps efficient to get values of all variables together? [similar to JDI's field values, var values APIs]
> Efficient in what sense?  The implementation would be a convenience method filling the map.

Because variable read is from another process (as much as I read the 
implementation), we could probably many variables together and send over 
wire - rather than reading one variable at a time.

>> * JShell shotdown's doc says this:
>>
>> " This occurs either because the client process has ended (e.g. called System.exit(0)) or the connection has been shutdown, as by close(). Each call adds a new subscription."
>>
>> So, a separate process for execution is hinted in this method -- but I don't remember reading a summary of execution model. i.e., javac for compilation but execution is out of process via JDI. Perhaps that may be documented in summary?
> I’ll address that.
>
>> * JShell's unresolved method:
>>
>> http://cr.openjdk.java.net/~rfield/doc/jdk/jshell/JShell.html#unresolved-jdk.jshell.DeclarationSnippet-
>>
>> / For corralled <http://cr.openjdk.java.net/%7Erfield/doc/jdk/jshell/Snippet.Status.html#ACTIVE_CORRALLED> or |ACTIVE_FAILED| <http://cr.openjdk.java.net/%7Erfield/doc/jdk/jshell/Snippet.Status.html#ACTIVE_FAILED> declarations,..../
>>
>> Perhaps ACTIVE_CORRALLED or ACTIVE_FAILED ?
> Not parsing that.

"corralled" is linked to ACTIVE_CORRALLED enum  value. The doc says 
"coralled or ACTIVE_FAILED".  I thought it is better if it reads 
ACTIVE_CORRALLED or ACTIVE_FAILED. i.e., use enum constant name for both 
parts of "or".

Thanks,
-Sundar

>
> Thanks much for the review,
> Robert
>
>> Thanks
>> -Sundar
>>
>> On Friday 24 July 2015 07:38 AM, Robert Field wrote:
>>> Concerns from the first round of reviews have been addressed in an extensive overhaul.  Please review the new API --
>>>
>>>     http://cr.openjdk.java.net/~rfield/doc/
>>>
>>> All comments welcome.
>>> Note: I will be on vacation until August 11th, with limited internet so there may be a delay in answering questions.
>>>
>>> Thank you,
>>> Robert
>>>
>