Kulla: JShell API ready for round two review

[Robert Field]

> On Jul 28, 2015, at 9:15 PM, A. Sundararajan <sundararajan.athijegannathan at oracle.com> wrote:
> 
> 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?

It needs to be visible to the jshell tool, so it needs to be public.  See my response to Brian.  If it is non-exported, I assume the tool could still see it?  Oh, and there is some annotation or tag to hide the docs, isn’t there?

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

I hacked the doc generation to allow the @jls tag, maybe I need something for <cite>

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

Hmmm, so I could add it to the API now with the dumb implementation and then be able to add the optimization in the future.

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

True, more consistent.  Will change.

Thanks a lot,
Robert

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