RFR 8170162: jshell tool: no mechanism to programmatically launch

[Robert Field]

Updated per review --

Webrevs:

     http://cr.openjdk.java.net/~rfield/8170162v1.webrev/

     http://cr.openjdk.java.net/~rfield/8170195v1.webrev/

API:

     http://cr.openjdk.java.net/~rfield/8170162v1.jshellAPI/

SpecDiff:

http://cr.openjdk.java.net/~rfield/8170162v1.specdiff/overview-summary.html

References in-line...

On 12/07/16 20:26, Robert Field wrote:
> Thank you for the review, Jon!
>
> Responses in-line below.
>
> On 12/07/16 18:50, Jonathan Gibbons wrote:
>> Many of the new files in the langtools webrev do not have the 
>> correct, required, legal header. This is a MUST-FIX,
>
> Oops!!

http://cr.openjdk.java.net/~rfield/8170162v1.webrev/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/JShellToolBuilder.java.html
http://cr.openjdk.java.net/~rfield/8170162v1.webrev/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/JShellToolProvider.java.html
http://cr.openjdk.java.net/~rfield/8170162v1.webrev/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/PersistentStorage.java.html

>
>>
>> In the new package-info file, this comment is a little bit hokey 
>> and/or out of place.
>>
>>   52  * Note: The Java™ shell tool is built on the {@link 
>> jdk.jshell} API,
>>   53  * but the tool is at a different level, and this launching SPI 
>> does not
>>   54  * interact with the {@link jdk.jshell} API.
>
> Not sure what is hokey, but the additional documentation I added to 
> overview,html clarifies the relationship between the layers more 
> clearly and is more appropriately placed than this, so, I will remove.

http://cr.openjdk.java.net/~rfield/8170162v1.jshellAPI/jdk/jshell/tool/package-summary.html

http://cr.openjdk.java.net/~rfield/8170162v1.jshellAPI/jdk.jshell-summary.html

> BTW: Seems I should dump overview.html, putting that in module-info.java?

Removed --

http://cr.openjdk.java.net/~rfield/8170195v1.webrev/make/Javadoc.gmk.sdiff.html

http://cr.openjdk.java.net/~rfield/8170162v1.webrev/


>
>>
>> At a minimum, I suggest making it an @apiNote, but I would recommend 
>> trying to rethink/reconsider
>> what you are trying to say here.
>>
>> I think you should figure a way and place to specify the non-null 
>> name used by the standard implementation
>> of the JShellToolProvider. That being said ....
>
> "The name of the standard implementation is 'jshell'" ???
> In jdk.jshell.tool.JavaShellToolProvider.name()

http://cr.openjdk.java.net/~rfield/8170162v1.jshellAPI/jdk/jshell/tool/JavaShellToolProvider.html

>
>>
>> I guess I'm surprised you need to define a JShellToolProvider PAI -- 
>> do you expect there to be multiple
>> implementations of JShell on any system?
>
> Maybe not.  But who knows, we already have an external fork..
> I suppose we could have experimental versions in incubator modules...
> I could also imagine a future with the jshell tool implementation 
> broken into a separate module and slimmed-down runtime images built.
>
>>   Why not just have
>>     static JShellToolProvider.instance()
>> to return an instance of the one true jshell implementation?
>
> Certainly an option (depending on the above).  Though the difference 
> in interface is:
>
>     findFirst(String)
>     name()
>
> vs
>
>     instance()
>
> So, we get future-proofing almost for free.

No changes made here until we come to a fixed-point.

Note: an additional option, rather than "instance()" is a static 
"builder()" method on the provider since "instance().builder()" is the 
only interesting thind you could do with it.   Could add while leaving 
or removing the other methods.

Thanks,
Robert

>
> The interfaces and implementation classes are separate, but I'd want 
> this anyhow, since I very intentionally have them in separate packages.
>
> -Robert
>
>
>>
>> -- Jon
>>
>>
>> On 11/26/2016 02:19 PM, Robert Field wrote:
>>> Goldilocks and the three launching mechanisms...
>>>
>>> She lay down in the first bed, but it was too hard.
>>>     java.util.spi.ToolProvider (8169821) has a severe impedance 
>>> mismatch: the use of Writers, no InputStream, and no flexibility.
>>>
>>> Then she lay in the second bed, but it was too soft.
>>>     javax.tools.ToolProvider (8170044) would require significant 
>>> changes that are not possible at this time, it is limited, and is 
>>> old mechanism.
>>>
>>> Then she lay down in the third bed and it was just right. Goldilocks 
>>> fell asleep.
>>>     jdk.jshell.tool (8170162) provides a matching, clean, and 
>>> configurable launching mechanism
>>>
>>> Please review...
>>>
>>> Bug:
>>>
>>>     8170162: jshell tool: no mechanism to programmatically launch
>>>     https://bugs.openjdk.java.net/browse/JDK-8170162
>>>
>>> Sub-Tasks (tiny tweaks to other repos):
>>>
>>>     8170194: jshell tool (jdk repo): launch tool from 
>>> JShellToolProvider
>>>     https://bugs.openjdk.java.net/browse/JDK-8170194
>>>
>>>     8170195: jshell tool (make): update javadoc generation for 
>>> jdk.jshell
>>>     https://bugs.openjdk.java.net/browse/JDK-8170195
>>>
>>> Webrevs:
>>>
>>>     http://cr.openjdk.java.net/~rfield/8170162v0.webrev/
>>>
>>>     http://cr.openjdk.java.net/~rfield/8170194v0.webrev/
>>>
>>>     http://cr.openjdk.java.net/~rfield/8170195v0.webrev/
>>>
>>> API:
>>>
>>>     http://cr.openjdk.java.net/~rfield/8170162v0.jshellAPI/
>>>
>>> SpecDiff:
>>>
>>> http://cr.openjdk.java.net/~rfield/8170162v0.specdiff/overview-summary.html 
>>>
>>>
>>> Thanks,
>>> Robert
>>>
>>
>