search

knime-ip / knip-scijava

KNIP - SciJava Commands Plugin
2 stars 3 forks source link

Use SciJava Converters #2

Closed dietzc closed 8 years ago

dietzc commented 9 years ago

Hi @Squareys. While beeing sick at home I had an idea: Couldn't we actually use the SciJava Converter framework for the mapping of KNIME DataCells to the values required by the Plugin?

See: https://github.com/scijava/scijava-common/blob/master/src/main/java/org/scijava/convert/Converter.java

Squareys commented 9 years ago

Yes, we can. I think @ctrueden mentioned that at the hackaton aswell. Just didn't get around to doing that yet.

Squareys commented 9 years ago

I have begun refactoring to resolve this. See adapter-refactor branch. Currently OutputAdapters are now Converters, but for two reasons I suggest keeping the Adapter-Framework:

  1. Performance: it must be faster to only have to check in a subset of converters
  2. Get All Adapters: This method is required by the @Parameter code creation in the script editor. We need to determine to what type a DataValue can be converted.

The second one is the more important one in my opinion, if that is not actually true, and there is a way to get all Classes to which a DataValue can be converted in the ConvertService, I will let go of the Adapter Framework and make everything pure converters.

dietzc commented 9 years ago

the current matching strategy is not really working. we should really use the Converter API of SciJava as this API exactly fits our needs (hopefully even better in the future).

However, I don't exactly understand the second point. There should be a way to get all possible converters. But I don't understand why we need this? Because we want to support the selection in the dialog?

ctrueden commented 9 years ago

@dietzc I think the point is that you can quickly retrieve all Adapter plugins, without the list being polluted with all non-Adapter Converters. This should be possible if the Adapter interface is a subtype of Converter, and plugins are annotated with @Plugin(type = Adapter.class). I see no harm in specializing the adapters that way—they are still Converters with all the benefits that brings.

I didn't have time to check the adapter-refactor branch yet, sorry, so can't comment on whether a separate "adapter framework" is needed here. But I am of course skeptical.

if ... there is a way to get all Classes to which a DataValue can be converted in the ConvertService

We could certainly add that. There is already a sort of inverse: for a given destination type, you can ask "what are all the known objects which could successfully be converted to that type"? This is handy in a number of circumstances.

dietzc commented 9 years ago

@dietzc I think the point is that you can quickly retrieve all Adapter plugins, without the list being polluted with all non-Adapter Converters. This should be possible if the Adapter interface is a subtype of Converter, and plugins are annotated with @Plugin(type = Adapter.class). I see no harm in specializing the adapters that way—they are still Converters with all the benefits that brings. I didn't have time to check the adapter-refactor branch yet, sorry, so can't comment on whether a separate "adapter framework" is needed here. But I am of course skeptical.

I was thinking the same, that's why I was wondering.

We could certainly add that. There is already a sort of inverse: for a given destination type, you can ask "what are all the known objects which could successfully be converted to that type"? This is handy in a number of circumstances.

We will add this functionality in a PR or open an issue if we really need it.

Thanks keeping an eye on this project @ctrueden!

Squareys commented 8 years ago

Done with the merge of #5.