Kulla: JShell API ready for round two review

[Robert Field]

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

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

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

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

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