[WIP] Refactoring and cleanup

[Squareys]

Hello everybody!

This is @dietzc's work-in-progress which I will be doing a polishing pass on.

TODOs:

• [x] Documentation
• [x] Batch together WIP commits with some description of what was done
• [x] Repair all FIXMEs
• [x] Proper Logging / Exception handling (which is part of FIXMEs)
• [x] Proper MissingValue Handling
• [ ] Unit and Workflow-Tests where suitable
• [ ] Split scripting away, open PR in knip-scripting and move it there.
• [ ] Remove temp ScijavaTestCommand and Script directory, implement a replacement

Regards, Jonathan.

This is a work-in-progress pull request and not meant to be merged before [WIP] is removed from the title!

[Squareys]

@dietzc I had this idea to split NodeModule and DefaultNodeModuleService into Scripting- and Command- to handle the slight differences (primarily "result" output module item atm). This could even be extended to more module info types than just ScriptInfo, CommandInfo. Possibly ImageJ Plugins?

My current attempt will not work, though, since the appropriate Service cannot be chosen depending on the module info type. Instead, I would introduce a new singleton plugin type which allows adding support for more ModuleInfo types in DefaultNodeModuleService (which will then handle those plugins). It would basically define two methods createNodeModule() and createOutputColumnSpecs() (possibly Class<? extends ModuleInfo) getModuleInfoType()).

Does that sound sane?

[ctrueden]

Why does KNIME need to handle the "result" output specially?

[dietzc]

@ctrueden each Script outputs a result String for logging errors etc. I think @gab1one and you have already discussed this issue and also solved it? The reason why we have to treat the result output in a a special way, is that in KNIME we create the output-spec of the resulting table (especially how many columns, which types etc) before we execute the node. However, the result is a special thing in scripting which is not part of the ModuleInfo we get after parsing the script. That's why we have to treat the result in a special way.

Either, we handle the result on our side or on scijava-side. I'd prefer to do it in scijava and if you have suggestions how to do it, I'm sure @Squareys can help you. It would be great if the abstraction layer we use in our code is ModuleInfo rather than having special cases for different types of ModuleInfo.

One related issue: https://github.com/scijava/scijava-common/issues/225

[ctrueden]

Ok great, I thought he was talking about the Object result which SciJava adds to scripts with no declared outputs.

[dietzc]

Actually, this one might be problematic, too. Is it also added to the ScriptInfo?

[dietzc]

After discussing this with @ctrueden in person we figured that we still should special case the result output of a ScriptInfo if it was added by the framework. Especially, as we want to support n-to-0 nodes. However, we don't need two implementations in this case., I think a if(info instanceof ScriptInfo) is fine.

[ctrueden]

More specifically, I would recommend:

boolean hasSyntheticResult = info instanceof ScriptInfo &&
    ((ScriptInfo) info).isReturnValueAppended();