Improve array -> `String` conversions from `DefaultConverter`

[gselzer]

This PR fixes the issue described in imagej/napari-imagej#65 by improving the conversion of arrays to Strings.

We make a few changes:

• Add case logic into DefaultConverter for the conversion from array types to Strings. Previously, for an array the conversion would output Object.toString(), which could not be converted back into the array. e.g. for new double[] {1, 2, 3}, we'd get something like [D@1234567. Now we get 1.0,2.0,3.0.
• Add functionality in ArrayUtils.toCollection to split a String by a delimiter (which is always just "," :sweat_smile:) before shoving it into an ObjectArray. This matches previous behavior when there are no commas, but would break previous behavior if there are commas...

Concerns:

• Is the converted String specific enough? Do we need to add the component type into the String somehow?
• Might this break backwards compatibility, for either change?
• Can/should we test more types?

This should be enough to close imagej/napari-imagej#65 once this change makes its way into a release.

[gselzer]

On second thought, why do we shove so much case logic into DefaultConverter? Why aren't these all split into separate Converters?

[ctrueden]

why do we shove so much case logic into DefaultConverter? Why aren't these all split into separate Converters?

Hysterical raisins. There didn't used to be modular converters, and at one point I split a bunch of logic into separate plugins, but was not able to fully split everything. Please feel free to do it, as your time allows!

[gselzer]

Sounds good. It's Priority.EXTREMELY_LOW as far as I'm concerned, so maybe I'll just make an issue about it.

[ctrueden]

@gselzer To answer your specific questions:

Is the converted String specific enough? Do we need to add the component type into the String somehow?

I think it is OK not to. The conversion is allowed to be lossy. If we baked in the component type of the array, it would make reconstruction from string more rigorous, but also more complicated. It would also make converted arrays harder for humans to read.

Might this break backwards compatibility, for either change?

I don't think it breaks backwards compatibility in a broad sense, but specifically for strings passed to ArrayUtils.toCollection, it does. Previously, strings would be wrapped into singleton collections. We should not change this behavior.

Can/should we test more types?

I pointed out some cases where the conversion is currently problematic. We should test those cases. In particular, for nested arrays, and strings containing special characters that need escaping, we should test.

[gselzer]

I'm not sure we want to do this.

Do you mean that we might want to close the PR?

If we do, then I think we need a more robust stringification of the array data. In that case, I would recommend escaping each string into a YAML-compliant form, since YAML solves essentially the same problem of how to express a properly typed array in string form.

I'm not sure that I understand what you mean by a "YAML-compliant form". Can you elaborate on what you mean? I do agree, looking back, that we should probably be using some sort of brackets if possible; I'm not sure why I did not do that before.

Is the converted String specific enough? Do we need to add the component type into the String somehow?

I think it is OK not to. The conversion is allowed to be lossy. If we baked in the component type of the array, it would make reconstruction from string more rigorous, but also more complicated. It would also make converted arrays harder for humans to read.

:+1:

Might this break backwards compatibility, for either change?

I don't think it breaks backwards compatibility in a broad sense, but specifically for strings passed to ArrayUtils.toCollection, it does. Previously, strings would be wrapped into singleton collections. We should not change this behavior.

Yeah, you are right. I removed the changes made to that method.

Can/should we test more types?

I pointed out some cases where the conversion is currently problematic. We should test those cases. In particular, for nested arrays, and strings containing special characters that need escaping, we should test.

Thanks, I tested nested arrays, empty arrays, is there anything else that we should be testing?

[gselzer]

Thanks again for taking a look @ctrueden

[gselzer]

@ctrueden I think we are in a much better place now with Parsington, thanks for the suggestion!

[ctrueden]

@gselzer I added a couple more asserts, checking that the result strings converted from arrays are what we expect, before the tests continue and convert them back into arrays again. I wanted to be totally sure that strings with more challenging characters are escaped correctly.

[ctrueden]

@gselzer Turns out there were still problems after I merged this PR. Now fixed in 42db182fa90ebe4fc79163713b8cf5ac7ee9b143 and 497eff6eca3ee9687acddd9453859fef40071390.

[gselzer]

Thanks for those fixes @ctrueden. Admittedly I went through and made the changes pretty quick.