Kulla: JShell API ready for round two review

[A. Sundararajan]

ErraneousSnippets have associated error diagnostics list (as a private 
field in super class Snippet). Can that be exposed as a method? There is 
a JShell method "diagnostics" to get any given Snippet -- but that would 
diagnostics only for ErroneousSnippet only (?).  Perhaps this belongs to 
ErraneousSnippet?

Thanks,
-Sundar

On Friday 31 July 2015 06:34 AM, Robert Field wrote:
>> On Jul 30, 2015, at 1:37 PM, Brian Goetz <brian.goetz at oracle.com> wrote:
>>
>> Overall, the move to source-based snippets is a big step forward for the API.
>>
>> Some minor changes suggested:
>>
>> 1.  Creation.  The builder() approach is fine; I would pair this with a convenience method JShell.create() which builds a JShell with the default parameters.
> So, you are saying you want:
>
>      JShell.create()
>
> as a convenience method for:
>
>      JShell.builder().build()
>
> ?
>
> Documenting the latter build approach as the standard way to create an instance exposes the builder without throwing it in there face.  Seem to me that adding that the one method call shorter approach makes thing more confusing: “so, do I use create() or builder()?!?".
>
>> 2.  Missing JShell methods to inspect and/or remove from class path.
> Yep.  There is a JBS issue for that: 8129539 <https://bugs.openjdk.java.net/browse/JDK-8129539>
>
> Should I stub it out so it is in the API now?
>
>> 3.  In order for classes to be immutable, you must also restrict subclassing.  Otherwise someone could subclass MethodSnippet and add mutable state.  (You can control subclassing by having a package-protected constructor.)
> All the snippets classes already have package-protected constructors.
>
>> 4.  With the type hierarchy as it is, can we now get rid of SubKind? Can't we add isXxx methods onto the right subclasses of Snippet? (ImportSnippet.isSingleStaticImport(), isSingleTypeImport(), etc.)  I think this depends a bit on usage scenarios, but I assume you're not testing to see if subkind is TEMP_VAR_EXPRESSION unless you already know its an expression?
> On the contrary, the jshell tool, and I suspect any tool giving user feedback, switches on SubKind not Kind. As a couple of random examples, the descriptive output for a temp-var creation (which to the user is just an expression) is quite different from that of a user created variable (even though they are the same Kind).  And class creation is described as “class X” vs “interface X” or “enum X”
>
>> 5.  Does ErroneousSnippet need to be a public type?  It has no methods or fields of its own.  I would think simply returning a Snippet whose kind is ERRONEOUS is fine.
> True, there is no information on ErroneousSnippet.  I’m concerned by the asymmetry of make one exception to one-to-one Kind — *Snippet pairing.
>
>> 6.  What is InternalDebugControl?  Will it eventually not be public?  If it is to be public, consider moving its functionality to JShell.
> It is how internal debugging is turned on by the jshell tool.  Which is my main way of debugging the API.  I don’t want it public but the jshell tool is just a client of the API. Hence my question on how to hide it.  Sundar suggests non-exported class.
>
>> 7.  SCA.SuggestionKind -> only has two kinds.  Perhaps replace with a boolean (or numeric) indicator of quality?
> Sounds reasonable to me.  Jan?
>
>>
>> Also, naming is a bit all over the map.  Some suggested naming changes:
> Created: 8132752 <https://bugs.openjdk.java.net/browse/JDK-8132752>
>
>> JShell.unresolved(snippet) is a confusing name, because it neither has a verb nor does it describe a quantity (as "methods" and "variables" do.)  Perhaps: unresolvedDependencies()?
> Sounds good.
>
>> The use of the term "type" as an umbrella for classes, interfaces, annotations, and enums is accurate, but may not be familiar to users.  I think "classes" will be more familiar -- and, since all of these source-level entities compile into CLASSfiles, I don't think its inaccurate.  (This shows up in a lot of places; "types()", "TYPE_DECL", etc.)
> Again, remember the “users” of the API: they are API developers and tool hackers.  They know what a type is.
>
> End users of the jshell tool or other tools built from the API do not see these names.
>
> Everywhere in the API naming and documentation is tied to the JLS.  To use the JVM naming in one place, ClassSnippet, is confusing, esp. since as a SubKind they are split out.
>
>> UnresolvedException -> UnresolvedXxxException, for some Xxx.
> UnresolvedReferenceException ?
>
>> XxxDeclSnippet -> XxxSnippet
> That introduces a slight ambiguity, but it is cumbersome, OK I’ll remove.
>
>> onSnippetStatusChange -> onStatusChange or onSnippetEvent
>>
>> SnippetStatusEvent -> SnippetEvent
> These were changed based on previous requests for being more descriptive and consistent.
>
> Is there a reference to existing (well named API) or documentation?
>
> Thanks much for the review,
> Robert
>
>>
>>
>>
>> On 7/23/2015 10:08 PM, 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
>>>