Add Crosswalk instances for: NonEmpty, Proxy, Const, Functor.Sum, These1 #193
Conversation
mniip
changed the title
|
@phadej Thoughts? |
|
@phadej gentle prod? |
|
Do you disagree with some of the instances? Would it help to split this into 6 pull requests? |
semialign/src/Data/Crosswalk.hs
Outdated
| Fill deff fs <*> Fill defx xs | ||
| = Fill (deff defx) (alignWith (uncurry id . fromThese deff defx) fs xs) | ||
|
|
||
| instance Traversable t => Crosswalk (MaybeT t) where |
There was a problem hiding this comment.
This doesn't look right (even the tests pass). MaybeT t is essentially Compose t Maybe, and I't expect the instance to be exactly that.
There was a problem hiding this comment.
If we look at sequenceL @Maybe as a sort of generalization of catMaybes (that returns Nothing if input was all Nothings), then Crosswalk (Compose f g) first removes Nothings from g's, and if that removed all Nothings, it then removes the g altogether from f, using Crosswalk f.
Crosswalk (MaybeT f) doesnt do this last step, hence being traversable is sufficient.
There was a problem hiding this comment.
So Crosswalk (MaybeT t) behaves differently than Compose t Maybe? If it's so, I'd rather not add this instance at all.
There was a problem hiding this comment.
Yes. It doesn't have to be on the MaybeT type constructor specifically (could be a newtype wrapper), but an instance that behaves like this is immensely useful, because it allows implementing sequenceL for any type that can implement catMaybes (witherable):
sequenceL @t xs = catMaybes @t . runMaybeT <$> sequenceL @(MaybeT t) (MaybeT (Just <$> xs))There was a problem hiding this comment.
Are you claiming that every Witherable is also Crosswalk?
There was a problem hiding this comment.
If it's so, I'd rather not add this instance at all.
There was a problem hiding this comment.
I've removed it from this PR, maybe let's have a separate discussion about it.
mniip
changed the title
No description provided.