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());
> I preferred to leave jclouds-karaf clean, and add this ugly workaround in
> the CLI.
That makes sense for me - it's the CLI that is doing the "odd" thing by
manually instantiating the commands in the first place, as you describe above.
Looking at the code, we've also added this change in the place where we are
instantiating those objects, which also makes sense to me.
The more minor questions I would still have are:
* `new ScriptEngineManager()` - do we need a new instance here each time, or
can an instance be shared?
* move the `if (action instanceof ComputeCommandBase) {...` block into a method
`configureShellTableFactoryIfNeeded` or so, just to reduce some of the noise?
* `if (factory.getScriptEngineManager() == null) {` - is that ever expected to
be **non**-`null`?
* add a comment describing a bit of what you wrote above? That helped me a lot
in understanding why we are doing this - hopefully others too ;-)
--
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
