-
Notifications
You must be signed in to change notification settings - Fork 375
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
Profunctor and bifunctor specification #137
Profunctor and bifunctor specification #137
Conversation
72e8514
to
4a03779
Compare
A value that implements the Profunctor specification must also implement | ||
the Functor specification. The Profunctor specification is a contravariant | ||
functor on its first type parameter and covariant functor on its second type | ||
parameter. |
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.
must also implement the Functor specification
Why is this the case for Profunctor
, but not (explicitly) for Bifunctor
? It made sense when Profunctor
was defined as map
+lmap
, but seems arbitrary now.
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.
It's probably not necessary given that it is mentioned in the second sentence now. I'm happy to remove 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.
If we want people to be able to assume that they can call map
on a profunctor or bifunctor, then we should add "must also implement the Functor specification" to the bifunctor spec as well. If not, we should remove it here for consistency.
Will it make sense to also add derivation of |
If we add the restriction that both Bifunctor and Profunctor require Functor, then I think it would be helpful to add the derivations of |
parameter. | ||
|
||
1. `p.promap(a => a, b => b)` is equivalent to `p` (identity) | ||
2. `p.promap(compose(f1)(f2), compose(g1)(g2))` is equivalent to `p.promap(f1, g1).promap(f2, g2)` (composition) |
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.
The order of composition seems wrong. Should be:
p.promap(compose(f1)(f2), compose(g2)(g1)) ≡ p.promap(f1, g1).promap(f2, g2)
Also we probably should use "inlined" function composition like in the rest of the spec, and I would use four letters instead of letters with numbers, so:
p.promap(x => f(g(x)), x => h(i(x))) ≡ p.promap(f, i).promap(g, h)
I took letters from https://hackage.haskell.org/package/profunctors-5.2/docs/Data-Profunctor.html
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.
Good catch. ⚡
+1 from me for adding dependency on Functor and derivation rules of map to both types. This way we also make sure that if a type has hand written |
Sorry for all the nitpicking, I'm trying to be a consistency nazi. Hopefully this helps :) |
4a03779
to
cf17c9b
Compare
A value which satisfies the specification of a Profunctor does not need to | ||
implement: | ||
|
||
* Functor's `map`; derivable as `f => this.promap(a => 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.
this
works differently in arrow functions, we need to use function
here.
Otherwise everything looks good to me now.
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.
⚡
cf17c9b
to
66239c9
Compare
I believe this PR would have to be rewritten a little to comply with the changes made in #134. Specifically, the derivatives need to be moved down to their own chapter. |
As @Avaq pointed out, this pull request needs a few tweaks. |
I'll try to find some time to get it up to date this week. |
7850f63
to
30638f5
Compare
I've pushed up some changes to make it consistent with #134. |
- [`map`][] may be derived from [`promap`]: | ||
|
||
```js | ||
function(f) { return this.promap(a => 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.
We're missing a semicolon after the closing paren.
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.
Order has been restored :D ⚡
30638f5
to
1804cf0
Compare
I plan to define these methods for various data types. I'd love to see this pull request merged, as it's nice to have documentation point to the FL spec rather than an unmerged pull request. :) |
Any other objections from this being merged? ping @SimonRichardson, @joneshf |
👍 On Thu, 21 Jul 2016, 11:03 Scott Christopher, notifications@github.com
|
Thanks so much! |
🎉 |
@@ -9,5 +9,7 @@ module.exports = { | |||
sequence: 'sequence', | |||
chain: 'chain', | |||
extend: 'extend', | |||
extract: 'extract' | |||
extract: 'extract', | |||
bimap: 'bimap', |
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.
@SimonRichardson Could you release a new version on NPM that would include this changes to index.js
?
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.
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!
A slight variation of the profunctor and bifunctor specs raised in #124, removing the mention of
lmap
and renamingdimap
topromap
.