Kulla: JShell API ready for round two review

[Brian Goetz]

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.

2.  Missing JShell methods to inspect and/or remove from class path.

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

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?

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.

6.  What is InternalDebugControl?  Will it eventually not be public?  If 
it is to be public, consider moving its functionality to JShell.

7.  SCA.SuggestionKind -> only has two kinds.  Perhaps replace with a 
boolean (or numeric) indicator of quality?


Also, naming is a bit all over the map.  Some suggested naming changes:

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

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

UnresolvedException -> UnresolvedXxxException, for some Xxx.

XxxDeclSnippet -> XxxSnippet

onSnippetStatusChange -> onStatusChange or onSnippetEvent

SnippetStatusEvent -> SnippetEvent




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
>