-
Notifications
You must be signed in to change notification settings - Fork 379
have traverse take a type representative rather than an "of" function #220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
``` | ||
|
||
A value which has a Traversable must provide a `traverse` method. The `traverse` | ||
method takes two arguments: | ||
|
||
u.traverse(f, of) | ||
u.traverse(A, f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So HAPPY right now that the order was finally switched. 🎈 🍰 🎉 🌈 🦄
EDIT:
Well could potentially, Maybe be switched.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't like the old order, but I couldn't justify a breaking change to scratch my itch. But if we're making a breaking change anyway we can flip the parameters while we're at it. :)
Out of interest, why do you prefer the new order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well for me, the TR is the more stable of the two parameters. It defines a classification if you will.
I could have a whole family of a -> f b
functions, but they all tie to one f
.
So using some point-free traverse
, it just seems natural to fix that parameter and get a function back for that fixed Applicative. If that makes sense?
EDIT:
(I am not really that good at this stuff, so my reasoning may be 💯 🗑)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm with you 💯 per cent. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, except the identity law
const u = [x]; | ||
|
||
const a = u[traverse](Id[of], Id[of]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this old version, it should have been:
const a = u[traverse](Id[of], Id[of]);
const b = t(u);
and because of that, new version should be like this:
const identityʹ = T => eq => x => {
const u = [x];
const a = u[traverse](T[of], T);
const b = T[of](u);
return eq(a, b);
}
and on caller side:
test(traversable.identity(Id)(equality))
Array.prototype[fl.map] = Array.prototype.map; | ||
Array.prototype[fl.map] = function(f) { | ||
return this.map(x => f(x)); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to mutating globally defined prototypes and featuring bugs such as the one above, this file contains a hack for Array#fantasy-land/equals
, convincing me that the laws do not belong in this repository (#174). We really need sanctuary-type-classes to avoid monkey patching.
Sanctuary's property-based tests are really nice. See, for example, Functor.js. I'd like to move these tests to fantasyland/fantasy-laws and remove the laws directory from this repository. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidchambers DO IT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. :)
Sanctuary is missing some of the laws, as we've only defined tests for those applicable to Maybe and/or Either. sanctuary-js/sanctuary#323 adds tests for Alt, Plus, and Alternative. Once that pull request has been merged I'll create fantasyland/fantasy-laws and submit an initial pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
672b4db
to
498e823
Compare
Closes #207
I intend to merge sanctuary-js/sanctuary-type-classes#28 regardless, but it would be nice to make this improvement here too. :)