Consistent flattening behaviour for empty slices in structs

[dokterbob]

Test demonstrating #330 and a proposed solution to it consistent with (and copied/borrowed from) Go's own encoding/json package:

1. By default, an empty slice in a struct will flatten to "".
2. When the tag option omitempty is used, the field will be not be present at all.

Regrettably, I found the code in marshal_unmarshal_test.go inaccessible. To respect my time and sanity and as a suggestion to the maintainer of radix, not withstanding my gratitude to them for a great library, I have created a separate testify test suite for the Flatten() function.

[mediocregopher]

Hi @dokterbob , I'm afraid that in its current state this PR isn't a change I'd like to make to radix. The mapping of an empty array to an empty string is not something I think others would find to be obvious, and would probably lead to someone accidentally setting an empty string into their dataset without realizing it. Honestly I think the better strategy might be (though I haven't put a ton of thought into it) to have Flatten return an error if flattening a slice as a struct field, so users don't accidentally run into this issue in the future. And probably to better document struct flattening in general.

As for omitempty, I'm more open to that if you want to open a separate PR for it. If you do so, can you update the FlatCmd example so users know it's an option?

[dokterbob]

Hi @mediocregopher,

Thanks for the feedback! Let me try and reflect this so we're both sure we're talking about the same thing.

Currently a struct containing a slice (e.g.[]struct but also []string) doesn't emit anything, yielding an uneven amount of arguments; a key is emitted but no value, which yields an invalid Redis command and thus a Redis error. I think we're clear on this.

You are suggesting to have radix return an error when an empty slice is flattened. However, this excludes the possibility of setting an empty value, e.g. when we intentionally want to store an empty slice []struct{} or []string{}, as discriminated from the field not existing. As you point out, this is not immediately evident, but it's also not evident that storing an empty slice, which seems a perfectly valid value, yields an error.

In addition, having the omitempty be required to avoid errors in this case doesn't seem obvious and might not fit user expectations. The default behaviour yielding an error, requiring an explicit option to prevent it doesn't seem ideal to me. Are you sure this is the way you want to go?

Whatever the outcome, I would suggest explicitly documenting this in FlatCmd and Flatten or linking from FlatCmd to Flatten.

I am working on some more issues getting radix v4 to work. Depending on how that goes I might be able to update the PR as per the outcome of this conversation.

[mediocregopher]

You are suggesting to have radix return an error when an empty slice is flattened.

I am suggesting that radix should return an error whenever any slice which is the value of a struct (or map) field is being flattened. For example:

type Foo struct {
    A []string
    B string
}

Flatten(Foo{
    A: []string{"This is fine"},
    B: "ok",
})
// ["A", "This is fine", "B", "ok"]

Flatten(Foo{
    A: []string{"this", "is"},
    B: "not"
})
// ["A", "this" ,"is", "B", "not"]

Flatten(Foo{
    A: []string{"this","A","is", "B", "worse"},
    B: "oof",
})
// ["A", "this", "A", "is", "B", "worse", "B", "oof" ]

You can see in the last example that, even ignoring the question of empty slices, it's very possible to get into situations where the flattened form becomes ambiguous if the value of a struct field is a slice.

So this is why I suggest that Flattening structs with slice/map fields should just be disallowed completely, as it forces users to go another route which is less ambiguous (such as writing their own resp (un)marshalers, or encoding to JSON).