-
Notifications
You must be signed in to change notification settings - Fork 373
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
Add Alt, Plus and Alternative specs #197
Conversation
the [Applicative](#applicative) and [Plus](#plus) specifications. | ||
|
||
1. `x.ap(f.alt(g))` is equivalent to `x.ap(f).alt(x.ap(g))` (distributivity) | ||
1. `M.pempty().ap(f)` is equivalent to `M.pempty()` (annihilation) |
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 purescript it's in different order and i think it's correct, so I have opened issue purescript-control#34
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.
Because of Applicative's interchange
law they are equivalent so i have sticked to purescript's version
Also we can use If you like |
make it consistant with purescript version, even tho they are equivalent because of Applicative's interchange law
👍 for |
Will change then! and if in future we add |
Well, this is a discussion for another day, but it's not really a subclass of |
- move patches and test structures into `internal` - `Compose` and patching of `Array` form `laws/traversable` - `equality`, `Sum` and patching of `String` form `id_test` - `Id` from `id` - rename `id_test` to `test` as it's not just testing `Id` but `Maybe` and `Sum`
The PR LGTM overall 👍 |
Just: _ => this, | ||
Nothing: () => b, | ||
}); | ||
}; |
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 wonder if we just import fantasy-options
, I know people don't want more dependencies, but I don't want to envision lots of Maybe
s out in the wild. Everyone different to each other...
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 is something that I have struggled to understand myself. There doesn't need to be multiple option/maybe/either implementations; and, I don't know why we can't specify what option/maybe/either is once and for all. Half of the ones out in the wild are broken anyway. Mostly they're not functors, but they claim to be. That's a worse situation than anything else. But the data types are simple and don't necessitate reinventing the wheel in every sub-community.
Not being able to speak about option/maybe/either also complicates the spec in ways that I think are more painful than forcing one data type onto people. Look at ChainRec
. As far as I understand types, you can't really implement it. In order for it to be implementable, you need higher rank polymorphism. HIgher rank polymorphism is something that very few languages/tools support–flow and typescript do not support it. I don't think we gain much as a spec by forcing higher rank polymorphism on people. It's been hard enough to communicate higher kinded polymorphism.
If we can just speak about option/maybe/either: we won't have an unimplementable algebra, ChainRec
will be easier to understand, and we can bring in more useful algebras–Filterable
, Witherable
, Choice
, CoChoice
, Decidable
to name a few.
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.
Here is my responce to this issue (scroll down)
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 sorry. I was venting. This PR is not the appropriate place for it. Do not let my comment stop progress of this PR. If it's something we are going to address, we should do it in another issue.
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 feel the same way though :-(
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 we can discuss this in #200
a[map](f)[alt](b[map](f)) | ||
); | ||
|
||
module.exports = {associativity, distributivity}; |
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.
👍
T[zero]() | ||
); | ||
|
||
module.exports = {distributivity, annihilation}; |
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.
👍
T[of](a) | ||
); | ||
|
||
module.exports = {leftCatch}; |
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.
👍
x[chain](f)[alt](y[chain](f)) | ||
); | ||
|
||
module.exports = {distributivity}; |
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.
👍
T[zero]() | ||
); | ||
|
||
module.exports = {annihilation}; |
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.
👍
T[zero]() | ||
); | ||
|
||
module.exports = {rightIdentity, leftIdentity, annihilation}; |
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.
👍
Just one comment otherwise 🎉 |
That's interesting point, suppose we depend on fantasy-id, fantasy-options, fantasy-compose ... What would happen if someone adds other spec? how do you see for example process of adding some spec Also |
Possibly solution is to completely remove |
one issue is that Maybe is not MonadPlus but is MonadOr mplus a b >>= f = mplus (a >>= f) (b >>= f)
a = Just 0
b = Just 1
f 0 = Nothing
f 1 = Just 1 so this will fail: test(x => monadPlus.distributivity(equality)(Maybe.Just(0))(Maybe.Just(1))(
a => a === 0 ? Maybe.Nothing : Maybe.Just(a)
)) Array is MonadPlus, but not MonadOr, so I have updated Array patch to test it against monadPlus laws. |
- fix Maybe equals - make Array Alternative and Monad - test Array for MonadPlus - test Maybe for MonadOr
At this point it's ready to be merged, But there is one thing, which I don't quite like: If some structure has I think it could be problematic in js (because of lack of type system), but it could also be problematic even in purescript? f :: (Monad___ m) => m a -> m a -> (a-> m b) -> m b
f m1 m2 g= (m1 `alt` b2) >>= g I don't have strong opinion on it, maybe it's not a big dial and it's actually ok, but wanted to point it out. Also I would love to hear what @garyb and @paf31 think on it as in PS there is not MonadOr (yet). -- update in MonadPlus reform proposal MonadPlus is defining
instance MonadPlus Maybe where
mplus (Just a) Nothing = Just a
mplus Nothing (Just a) = Just a
mplus _ _ = Nothing
instance MonadOr Maybe where
-- this is same as definition in Alternative
morelse (Just a) _ = Just a
morelse _ b = b as well as instance MonadOr [] where
morelse [] b = b
morelse a b = a
instance MonadPlus [] where
-- this is same as definition in Alternative
mplus = (++)
|
Is that true? It seems like you can have something tha is |
image was squashed because of `px` sizing.
@joneshf technically one could make So to be precise if structure is Applicative(of,ap) and Chain(chain) than we have two choices
if structure is Applicative(of,ap) Alternative(alt,zero) and Chain(chain) then we have 4 choices:
|
I wouldn't care too much about detection of supported algebras by presence of methods. If this will become a real problem we can solve it for example by requiring values to have But we should think about structures that may support both MonadPlus and MonadOr, as far as I understand. |
Exactly. In that case we should have separate methods for MonadPlus and MonadOr. in the haskell proposal names: if structure is MonadPlus or MonadOr it could define T.prototype[fl.mOrElse] = T.prototype[fl.alt] = function(a) { .... }
T.prototype[fl.mPlus] = T.prototype[fl.alt] = function(a) { .... } do others agree on the names? |
I think it's ready for final review and then it could be merged 🎉 🌮 |
@@ -25,7 +28,7 @@ structures: | |||
* [Bifunctor](#bifunctor) | |||
* [Profunctor](#profunctor) | |||
|
|||
<img src="figures/dependencies.png" width="863" height="347" /> | |||
<img src="figures/dependencies.png" width="100%" /> |
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.
Will fix that. We definitly need CONTRIBUTING.md
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.
on github the width of README is fixed but for example on npm it looks bad because max width of readme area is 727px and if you are on small screan than it's worth.
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.
Yeah, same issue with mobile version of GitHub for example. I wish we could find a fix without this side effect, but in the current situation having links to point to the right locations is more important imo.
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.
Ok will change it as it was before.
But we could move the image to the end of README and have a link in it's place where it was before (next to list of specs) and also a link back.
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.
+1 for moving to the end.
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.
you can check rendered image here
#### `zero` method | ||
|
||
```hs | ||
zero :: Plus x => () -> 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.
Should it be Plus x => () -> x a
? Also maybe use p
instead of 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 that case we should also add forall a
I think, but i'm not familiar with that. maybe @joneshf could help here?
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 forall
is kinda implicit in our loosely specified type system. What I was trying to say is that x
is not a type here, it's a type constructor, so it cannot be in that position in signature. x a
is a type on the other hand.
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 would use f a
. f
to be consistent with other types
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
@SimonRichardson @davidchambers I think it's ready to be merged ⏲ |
I'll check tomorrow if that's ok On Wed, 2 Nov 2016, 18:11 Irakli Safareli, notifications@github.com wrote:
|
⏰ a friendly reminder |
@safareli sorry, I looked at this yesterday and forgot to 👍 MERGE IT |
Would you mind squashing the commits, @safareli? It would be nice to avoid mentioning MonadZero, MonadOr, and MonadPlus in the commit history to avoid confusion. |
I think this commits might be useful in future for reference. And as I remember before my committee where
And for it's description we could just say that tests where refactored a bit:
if you want to just |
Thanks for updating the pull request's title and providing an appropriate commit message. :) Shall we release v2.1.0? |
I think yes 🎈 |
Yea, releasing a new version would be nice. I need it to add this algebras to static-land. |
* 'master' of github.com:fantasyland/fantasy-land: (29 commits) Version 2.1.0 Add Alt, Plus and Alternative specs (fantasyland#197) Use uppercase letters for Type representatives in laws (fantasyland#196) Fix id_test and argument order in laws (fantasyland#193) Version 2.0.0 Another go at updating dependencies (fantasyland#192) release: integrate xyz (fantasyland#191) test: remove unnecessary lambdas (fantasyland#190) require static methods to be defined on type representatives (fantasyland#180) lint: integrate ESLint (fantasyland#189) Enforce parametricity (fantasyland#184) readme: tweak signatures to indicate that methods are not curried (fantasyland#183) Fix reduce signature to not use currying (fantasyland#182) Link to dependent specifications (fantasyland#178) Add Fluture to the list of implementations (fantasyland#175) laws/functor: fix composition (fantasyland#173) laws/monad: fix leftIdentity (fantasyland#171) Minor version bump bower: add bower.json (fantasyland#159) Fix chainRec signature to not use currying (fantasyland#167) ...
Specs added:
MonadZeroMonadOrMonadPlusTODO:
laws/*
addmPlus
andmOrElse
?fixes #187
Monad{Zero,Or,Plus} are removed from this PR (reason)