add some Iterable tests (#2894)

[jsoizo]

This is an additional test to increase coverage of Iterable #2894 .
I will add a few more except for the ones with @deprecated annotations.

[nomisRev]

Looks great! Thank you @jsoizo 🙌

I will add a few more except for the ones with @deprecated annotations.

Do you want to add them in this PR, or another one? Whatever works best for you is fine ☺️

[nomisRev]

We don't use * imports in the Arrow codebase.

[jsoizo]

We don't use * imports in the Arrow codebase.

I've put that back in.

Do you want to add them in this PR, or another one?

I will create another one.

[jsoizo]

@nomisRev I have added some tests and kDocs, could you please review them again?
Also, I have a question about the crosswalk implementation and left a comment with a question.

[jsoizo]

I am not sure if this explanation is adequate and would be happy to receive a revised proposal.
The same goes for crosswalkMap and crosswalkNull.

[jsoizo]

If you pass emptyList() as the first argument of fold, the results of Ior.fromNullables and croswalkNull will also not be null, since bs is always non-null. If you expect croswalkNull to be null if Iterable<A> is empty or all results of applying the function f are null, it is better to pass null as the first argument of fold.
Furthermore, it appears that if we modify it as described, crosswalkNull will no longer return an empty list, so it would be a good idea to set the return type to NonEmptyList<B>? ?

[serras]

If I understand correctly, this function should:

• return null if (and only if) f(it) returns null for some component,
• return emptyList() if the Iterable is empty
• return non-empty for a non-empty list where evey application of f returns non-null.

Does this make sense? Does this coincide with the behavior here?

[jsoizo]

@serras Thank you for your reply.

Does this make sense?

I have the same understanding as you have written.

Does this coincide with the behavior here?

However, the behavior seems not to coincides with the understandings.
The first and third case of following tests fails.

class CrosswalkNullTest : FunSpec({
    test("return null if f(it) returns null") {
        val list = listOf(1, 2, 3)
        val result = list.crosswalkNull { null }
        // This test fails because result is emptyList()
        result shouldBe null
    }

    test("return emptyList() if Iterable is empty") {
        val list = emptyList<Int>()
        val result = list.crosswalkNull { it.toString() }
        // This test passes
        result shouldBe emptyList()
    }

    test("return non-empty if Iterable is non-empty and f(it) returns not-null") {
        val list = listOf(1, 2, 3)
        val result = list.crosswalkNull { it.toString() }
        // This test fails because result is listOf("3", "2", "1")
        result shouldBe listOf("1", "2", "3")
    }
})

[jsoizo]

The third argument to ior.fold, listOf(l) + r, makes the list appear to be in reverse order of the original list. I would think that r +l would be a better behavior, but what do you think?

f(a).align(bs) { ior ->
      ior.fold(
        { listOf(it) },
        ::identity,
        { l, r -> listOf(l) + r }
      )
    }

[serras]

I agree, this seems wrong to me too. WDYT, @nomisRev?

[serras]

@jsoizo I've talked with @nomisRev, and we've agreed that it makes sense to fix the different crosswalk implementations, based on your findings in this PR.