Improve tests and extend TableIOPlugin

scijava / scijava-plugins-io-table

[OBSOLETE] Merged with https://github.com/scijava/scijava-table

Other

0 stars 0 forks source link

Improve tests and extend TableIOPlugin #6

Closed frauzufall closed 4 years ago

frauzufall commented 4 years ago

This PR is addressing https://github.com/scijava/scijava-table/issues/22 and making the DefaultTableIOPlugin compatible with the latest scijava-table version.

Changes:

fix tests to test default behaviour & API options instead of changing parameters via reflection
read and write row and column headers by default
DefaultTableIOPlugin extends TableIOPlugin
removed the plugin parameters, making use of TableIOOptions instead
guess column types based on first data row

frauzufall commented 4 years ago

Thanks for the comments @imagejan!

About testing guessParser for Long and Float: I'm not sure how to do this right and I'm also not sure if the guessParser method works as expected there.

This test fails, because the String can and therefore will be interpreted as Integer:

assertEquals(3L, DefaultTableIOPlugin.guessParser("3L").apply("3L"));

Using a number too big for an Integer should work, I thought, but this also fails and I don't understand why:

assertEquals(36564573745634564L, DefaultTableIOPlugin.guessParser("36564573745634564L").apply("36564573745634564L"));

Here is the result:

java.lang.AssertionError: 
Expected :36564573745634564
Actual   :36564573745634564L

Similar problem with Float vs Double. Maybe I'm missing something very basic about how numbers are represented.

imagejan commented 4 years ago

@frauzufall wrote:

assertEquals(36564573745634564L, DefaultTableIOPlugin.guessParser("36564573745634564L").apply("36564573745634564L"));

36564573745634564L will be interpreted as a String because of the trailing L, no?

I added a slightly modified line:

https://github.com/scijava/scijava-plugins-io-table/blob/922e92590ddd7294e11578250d8c5481f20109a9/src/test/java/org/scijava/table/DefaultTableIOPluginTest.java#L258

... and it passes.

However, we now still have dead code with the Float.valueOf, as this one will never be reached because any decimal number can be interpreted by Double.valueOf here:

https://github.com/scijava/scijava-plugins-io-table/blob/922e92590ddd7294e11578250d8c5481f20109a9/src/main/java/org/scijava/table/DefaultTableIOPlugin.java#L255-L262

frauzufall commented 4 years ago

@imagejan Thanks! I removed the dead code. It will just always use double when guessing, that's fine, right? I'm sure the guessing can be further improved, but we probably need to release a new version of this repo too before uploading the most recent version of scijava-table to Fiji..