Skip to content

Conversation

@frauzufall
Copy link
Member

@frauzufall frauzufall commented May 10, 2020

As discussed here, I added a TableIOPlugin and a TableIOOptions class which allows the TableIOService to open and write a table with specific options:

TableIOOptions options = new TableIOOptions()
	.readColHeaders(true)
	.readRowHeaders(true)
	.separator('-');
Table<?, ?> data = tableIOService.open(tableFile, options);

Happy for further suggestions! Here's what's missing from my point of view:

  • a non-snapshot version from scijava-optional
  • formatter / parser options to either guess the entry types, use one type for the whole table or specific class types for specific rows (I'll continue with this)
  • the export command needs to be tested and polished to include more / all options

@tpietzsch I'd appreciate it a lot if you could have a look at TableIOOptions - is this aligned with how you would use the options? Also, at which point would you retrieve the values? Currently the whole options instance is passed to the TableIOPlugin and in the implementation I eventually retrieve the values..

I'm working in parallel on this branch in scijava-plugins-io-table which builds on this version of scijava-table and improves the tests there and now also has a new version of DefaultTableIOPlugin.

@frauzufall frauzufall requested review from imagejan and tpietzsch May 10, 2020 11:35
@frauzufall frauzufall changed the title Add options to TableIOService API #11 Add options to TableIOService API May 10, 2020
@frauzufall
Copy link
Member Author

@imagejan I had a look at the KNIME CSV reader options just now as you suggested. I like column delimiter and row delimiter more than separator and eol, I would rename them.

@frauzufall frauzufall assigned frauzufall and unassigned frauzufall May 10, 2020
@imagejan
Copy link
Member

Sorry for not getting back to this earlier.

@frauzufall as far as I can see, scijava-optional has now stabilized and is at 1.0.0.
Do you want me to test and review, or do you still want to change anything (rename options etc.)?

@frauzufall frauzufall force-pushed the tableioservice-options branch from 7ab4379 to beea7b6 Compare July 2, 2020 15:21
@frauzufall
Copy link
Member Author

@imagejan I made it use the stable scijava-optional version and renamed the options, it'd be great if you could review this!

@frauzufall frauzufall force-pushed the tableioservice-options branch from beea7b6 to ace2aaf Compare July 2, 2020 15:34
Copy link
Member

@imagejan imagejan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, @frauzufall, this looks great!

I just added a few minor comments, will test more extensively later today.

Copy link
Member

@imagejan imagejan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, @frauzufall, I added a few more comments regarding the DRYness of the tests. Let me know what you think. Otherwise, I think all is good to merge.

@frauzufall frauzufall marked this pull request as ready for review July 6, 2020 09:42
@imagejan imagejan merged commit a305794 into master Jul 6, 2020
@imagejan imagejan deleted the tableioservice-options branch July 6, 2020 11:44
@imagejan
Copy link
Member

imagejan commented Jul 6, 2020

Beautiful, thanks @frauzufall!

  • the export command needs to be tested and polished to include more / all options

We can do that later, after making some progress on scijava/scijava-plugins-io-table#6.

imagejan added a commit that referenced this pull request Jul 6, 2020
With merging of #14, the API changed significantly.
@frauzufall
Copy link
Member Author

I agree. Thanks for reviewing!!

ColumnTableIOOptions options = new ColumnTableIOOptions();
columnOptions.putIfAbsent(column, options);
options.formatter(String::valueOf).parser(getParser(type));
return setValue(columnOptionsKey, columnOptions);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not return this like in columnParser(), columnFormatter(), ... ?

@tpietzsch
Copy link
Member

@frauzufall not important, but for improved toString() of the Values classes you could add forEach implementations.
E.g., in TableIOOptions.Values:

@Override
public void forEach( final BiConsumer< String, Object > action )
{
	action.accept( readColHeadersKey, readColumnHeaders() );
	action.accept( writeColHeadersKey, writeColumnHeaders() );
	action.accept( writeRowHeadersKey, writeRowHeaders() );
	action.accept( columnDelimiterKey, columnDelimiter() );
	// etc...
}

Then, if you print option.values (e.g., inspecting it in the debugger), the default values will show up.
I.e., for a vanilla new TableIOOptions().values, no default option has been overridden. so printing it will give you
options.values = {}
while with the overridden forEach it will give something like
options.values = {readColHeaders = true [default], writeColHeaders = true [default], ...}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants