-
Notifications
You must be signed in to change notification settings - Fork 1
Add options to TableIOService API #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@imagejan I had a look at the KNIME CSV reader options just now as you suggested. I like |
|
Sorry for not getting back to this earlier. @frauzufall as far as I can see, |
7ab4379 to
beea7b6
Compare
|
@imagejan I made it use the stable |
* adds scijava-optional dependency
beea7b6 to
ace2aaf
Compare
imagejan
left a comment
There was a problem hiding this 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.
imagejan
left a comment
There was a problem hiding this 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.
|
Beautiful, thanks @frauzufall!
We can do that later, after making some progress on scijava/scijava-plugins-io-table#6. |
With merging of #14, the API changed significantly.
|
I agree. Thanks for reviewing!! |
| ColumnTableIOOptions options = new ColumnTableIOOptions(); | ||
| columnOptions.putIfAbsent(column, options); | ||
| options.formatter(String::valueOf).parser(getParser(type)); | ||
| return setValue(columnOptionsKey, columnOptions); |
There was a problem hiding this comment.
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(), ... ?
|
@frauzufall not important, but for improved @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 |
As discussed here, I added a
TableIOPluginand aTableIOOptionsclass which allows theTableIOServiceto open and write a table with specific options:Happy for further suggestions! Here's what's missing from my point of view:
@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 theTableIOPluginand 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-tableand improves the tests there and now also has a new version ofDefaultTableIOPlugin.