Profunctor and bifunctor specification #137
Conversation
scott-christopher
force-pushed
the
profunctor-bifunctor
branch
from
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.
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.
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.
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.
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.
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 :) |
scott-christopher
force-pushed
the
profunctor-bifunctor
branch
from
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.
this works differently in arrow functions, we need to use function here.
Otherwise everything looks good to me now.
scott-christopher
force-pushed
the
profunctor-bifunctor
branch
from
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. |
scott-christopher
force-pushed
the
profunctor-bifunctor
branch
2 times, most recently
from
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.
We're missing a semicolon after the closing paren.
There was a problem hiding this comment.
Order has been restored :D ⚡
scott-christopher
force-pushed
the
profunctor-bifunctor
branch
from
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.
@SimonRichardson Could you release a new version on NPM that would include this changes to index.js?
There was a problem hiding this comment.
A slight variation of the profunctor and bifunctor specs raised in #124, removing the mention of
lmapand renamingdimaptopromap.