demobox commented on this pull request.


> @@ -344,7 +349,17 @@ private void discoverCommands(CommandProcessorImpl 
> commandProcessor, ClassLoader
                         @Override
                         public Action createNewAction() {
                             try {
-                                return ((Class<? extends Action>) 
actionClass).newInstance();
+                               Action action = ((Class< ? extends Action>) 
actionClass).newInstance();
+                               if (action instanceof ComputeCommandBase) {
+                                   ShellTableFactory shellTableFactory = 
((ComputeCommandBase) action).getShellTableFactory();
+                                   if (shellTableFactory instanceof 
BasicShellTableFactory) {
+                                       BasicShellTableFactory factory = 
(BasicShellTableFactory) shellTableFactory;
+                                       if (factory.getScriptEngineManager() == 
null) {
+                                           factory.setScriptEngineManager(new 
ScriptEngineManager());

> That will always be null, but, it is an extra check that could save us some 
> debugging if we change things in the future (unlikely, but it's a cheap 
> check).

@nacx Unless I'm missing something, the check would ensure that we don't 
overwrite any ScriptEngineManagers that miraculously happen to be set. But I 
think the intention here is precisely to set an SEM explicitly, even if it were 
somehow already set? That way, I think we can at least be sure that all 
commands will be initialized the same way.

A comment along the lines of `setting ScriptEngineManager explicitly as it's 
expected to be null` or so might help make that clearer.

And yes, I think that can easily wait until after 2.0.0 ;-)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-cli/pull/35

Reply via email to