Use Location instead of String

frauzufall commented 4 years ago

This PR depends on the following PRs:

It uses Location instead of String for opening and saving. It also adapts to the change in TableIOOptions where the parser is now of type Function<String, ?> instead of Function<String, Object>.

imagejan commented 4 years ago

@frauzufall I amended your commit with the new release versions in pom.xml and force-pushed the branch.

Your second commit makes the tests for Integer and Long fail. What's your opinion, should we adjust the tests to reflect the now-desired behavior to always return Double? Or can we somehow make the entire DefaultTableIOPlugin more robust by first scanning each column entirely before guessing a format??

frauzufall commented 4 years ago

@imagejan what do you think? I would try today to make it smarter and guess based on the whole column, if that's not going too much on the cost of performance. We definitely don't want to change this behaviour later again, right..

imagejan commented 4 years ago

Parsing the entire table before guessing a column parser will be prohibitively costly for long tables. I could imaging parsing the first n rows to guess all column types, but we can leave this to a more elaborate implementation other than the default one.

So I suggest removing/adapting the tests that currently rely on Long/Integer return values. Since the caller can always specify a parser for each column explicitly, I think that is fine.

We might want to add more rigorous testing of different column types combined in a single table later, in a follow-up pull request.

imagejan commented 4 years ago

I adapted the tests and had to change DefaultTableIOPlugin to make the tests pass, since Double.valueOf(content) wasn't doing the replacement of Nan and infinity yet:

index a570165..85bb5ef 100644
--- a/src/main/java/org/scijava/table/DefaultTableIOPlugin.java
+++ b/src/main/java/org/scijava/table/DefaultTableIOPlugin.java
@@ -247,13 +247,14 @@ public class DefaultTableIOPlugin extends AbstractIOPlugin<Table> implements Tab
        return options.parser();
    }

-   static Function<String, Object> guessParser(String content) {
+   static Function<String, ?> guessParser(String content) {
        try {
-           Double.valueOf(content);
-           return s -> Double.valueOf(s
+           Function<String, ?> function = s -> Double.valueOf(s
                    .replace("infinity", "Infinity")
                    .replace("Nan", "NaN")
            );
+           function.apply(content);
+           return function;
        } catch(NumberFormatException ignored) {}
        if(content.equalsIgnoreCase("true")||content.equalsIgnoreCase("false")) {
            return Boolean::valueOf;

Force-pushed the branch again, and will merge if tests pass on Travis CI.

scijava / scijava-plugins-io-table

Use Location instead of String #7