Fix traversable laws

[rpominov]

See #128

Opening the PR to get this moving.

[rpominov]

It's not clear here what x is. Maybe we should rewrite it as "t(f.of(x)).map(a) is equivalent to t(f.of(x).map(a))"?

[rpominov]

Rewrote Compose via .prototype and checked that it works in Ramda repl: http://goo.gl/ExPdoo

Also GitHub for some reason hided previous comments on other parts of the diff, although I didn't touch those parts:

These comments still need attention.

[SimonRichardson]

@rpominov 👍 on the prototype stuff.

[CrossEye]

This looks great! Thanks.

[joneshf]

While we're in here fixing this. Can we change the requirement to traverse rather than sequence? With traverse you can derive map and reduce for free. But you can't do that with sequence.

[joneshf]

Oh, actually. I guess we don't even have the requirement that Traversable implement Foldable as well. Better add that while fixing this as well.

[rpominov]

While we're in here fixing this. Can we change the requirement to traverse rather than sequence?

We could, but that would be a breaking change, do we really want that?

With traverse you can derive map and reduce for free. But you can't do that with sequence.

We can derive traverse using sequence and map, and then we can derive reduce. So we only missing possibility of deriving map, reduce still can be derived.

Should we add instructions how reduce can be derived? It's kind of tricky and requires two helper types Const and Endo from what I understood from #68 (comment) . Do we want that in the spec?

On other hand using traverse will be consistent with Monad that uses chain and not join.

I'm very curious what @DrBoolean thinks about all this, since he originally brought Traversable and Foldable to the spec.

[DrBoolean]

Love that you got rid of Id! I was thinking since Id was the example in the spec, it'd be okay to reference, but no one, including myself, wanted to mix in implementation. I guess there still is implementation there with Compose, but it's better inline.

A traversable is a foldable functor, yes, but I don't see the reason to enforce foldable. I think it should just be known that one can derive it. (#68 (comment))

I'm cool with switching to traverse, I originally wrote (ported) it that way, but found it to be more complex and only defined that way for edge case performance reasons. That said, i hardly ever use sequence and reach for traverse myself.

[rpominov]

Love that you got rid of Id! I was thinking since Id was the example in the spec, it'd be okay to reference, but no one, including myself, wanted to mix in implementation.

Great, we wasn't sure if we can replace Id with any Applicative in #128

A traversable is a foldable functor, yes, but I don't see the reason to enforce foldable.

Seems like in other places in the spec when we can derive one algebra from another that algebra is enforced.

I'm cool with switching to traverse

+1 from me, and I can add this to PR, but still not sure if everybody want this, because this is a breaking change.

[DrBoolean]

As far as any applicative goes, it'd be interesting to search for a counter example. Perhaps Const will break it

[rpominov]

Perhaps Const will break it

No, it works with Const. u.map(Const.of).sequence(Const.of) will create a bunch of Const(empty()) which then will be concatenated (using ap) to a single Const(empty()); and Const.of(u) also will give us Const(empty()).

[DrBoolean]

word!

[DrBoolean]

As far as "breaking change", since traverse and sequence can be defined in terms of each other, I don't see it as much of an issue. Especially if we provide derived sequence: traverse(F.of, id)

[rpominov]

Made Traversable to depend on Foldable, and added derived reduce() (checked that it works here http://goo.gl/nW1bkB)

[DrBoolean]

I disagree that we should say it must implement the foldable spec. Even though something is definitely foldable if it's traversable, we don't depend on reduce and it confuses the work involved to make a type traversable. One absolutely must have map, but not fold or reduce

I feel providing a derivable version is enough to show it's foldable.

[rpominov]

I don't have a strong opinion on this, but I think we should try to keep spec consistent. As I understand, before Traversable and Foldable were added, the one of rules to decide where a dependency must be introduced was: if algebra B can be derived from algebra A, then A must depend on B. An example of such case is Chain depending on Apply.

This seems relevant to #132 and #134

[rpominov]

Maybe it would be better to replace sequence with traverse in another PR, so it wouldn't block other changes from this PR? I can open PR for traverse right after this is merged.

If that's ok, the open questions left here:

• Should we add the requirement that Traversable implement Foldable? (currently added)
• Is the derived reduce() I wrote fine?

[DrBoolean]

I'm good with this. The derived traverse does seem out of place now that we've talked it all out, but i suppose that is a separate PR.

I have to say, amazing job on derived reduce and the naturality law!

I think this is a terrific clarification of the spec. Thanks for all your work on this.

Everyone else cool to update this?

[SimonRichardson]

Looks good to me.

On Sat, 16 Apr 2016, 16:49 Brian Lonsdorf, notifications@github.com wrote:

I'm good with this. The derived traverse does seem out of place now that
we've talked it all out, but i suppose that is a separate PR.

I have to say, amazing job on derived reduce and the naturality law!

I think this is a terrific clarification of the spec. Thanks for all your
work on this.

Everyone else cool to update this?

—
You are receiving this because you commented.

Reply to this email directly or view it on GitHub
#131 (comment)

[rpominov]

Great! The PR that replaces sequence with traverse is coming soon.

[CrossEye]

✨