Skip to content
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

WIP: Combination of #1 and #2 #10

Open
wants to merge 34 commits into
base: develop
Choose a base branch
from
Open

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jan 11, 2020

Credit to @xplat for the heterogeneous patch idea for use with dmap.

danbornside and others added 10 commits October 8, 2019 13:24
Co-Authored-By: Alexandre Esteves <2335822+alexfmpe@users.noreply.github.com>
Do this without changing strictness using ~.
This makes it match the new PatchMapWithMove, and obviates
PatchDMapWithReset.
This isn't good enough. I think there is a bug in that we don't store
enough information in the two field: we need both inner patches with the
join so we can combine them together. In the non-dependent case, nothing
stops us from making this misake, but in the dependent case we would
need to use a `Category` instance on the inner move patches. We plainly
aren't combining two inner patches in the double move case, and the
types let us know.
@Ericson2314
Copy link
Member Author

As described in d72221c I think all PatchFooWithMove are wrong currently in the double-move case of their Semigroup instance.

module Data.Monoid.DecidablyEmpty where

class Monoid a => DecidablyEmpty a where
isNull :: a -> Bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

according to conventions this should be either null or isMempty ...

unsafePatchDMapWithMove, weakenPatchDMapWithMoveWith)
import Data.Patch.DMapWithMove as X
( PatchDMapWithMove, const2PatchDMapWithMoveWith, mapPatchDMapWithMove
, patchDMapWithMoveToPatchMapWithMoveWith
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sprained my tongue :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(i have no suggestions though)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use a guid

negateG _ = Proxy
_ ~~ _ = Proxy
negateG ~Proxy = Proxy
~Proxy ~~ ~Proxy = Proxy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this actually necessary to be in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope

type PatchTarget p :: *
class ( PatchHet p
, PatchSource p ~ PatchTarget p
) => Patch p where
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like the way you tied these two together, it should make life easier in a lot of cases.

apply (Identity a) _ = Just a

-- | 'Proxy' can be used as a 'Patch' that always fully replaces the value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment is wrong -- a 'Patch' that never alters the value?

:: forall k v
. (GCompare k, GShow k)
=> DMap k (NodeInfo k v)
-> Bool
validPatchDMapWithMove = not . null . validationErrorsForPatchDMapWithMove
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

has anyone ever used this? the sense of the test looks reversed to me ...

-- [ toAfter :=> Fixup_Delete ]
-- _ ->
[ toAfter :=> Fixup_Update (This $ From_Move $ fromBefore :=> (Flip $ p1 `o` p0))
, fromBefore :=> Fixup_Update (That $ To_Move $ toAfter :=> (p1 `o` p0))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p1 o p0 should be pulled out into a let to avoid recomputation

moveDMapKey :: GCompare k => k a -> k a -> PatchDMapWithMove k v
moveDMapKey
:: GCompare k
=> k a -> k a -> PatchDMapWithMove k (Proxy3 v)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can be generalized to any patchy Category pretty safely ...

-- | Move the value from the given key @k a@ to this key. The target key
-- should also have an entry in the patch giving the current key in
-- @_nodeInfo_from@, usually but not necessarily with @To_Delete@.
To_Move :: !(DSum k (p from)) -> To k p from
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be better actually, once the types have been verified as lining up, to force one of these to Proxy so there aren't two copies of the patch that can end up being unshared.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is what the original MapWithMove2 patch used to do, more or less. Maybe check out that PR. I couldn't get version to type check but perhaps I needed to just rebuild some values more to get things to type check.

-- This type isn't used directly as the from field patch, but is instead wrapped
-- in an existential. However, it is nice to be able to reason about this in
-- isolation as it is itself a @Semigroupoid@ when the underlying patch is.
data By (k :: a -> *) (p :: a -> a -> *) :: a -> a -> * where
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading this, how is it different from From?

Copy link
Member Author

@Ericson2314 Ericson2314 Jan 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only difference is no existentials and so has a Category instance.

It's not actually used at all; I made it in the process of figuring out what I was doing. I meant to call out that we probably should delete it but still keep it for the some review unless it helped anyone think about it too.

@Ericson2314
Copy link
Member Author

Should be made to include #14, and do something similar for a DMapWithMove vs DMapWithPatchingMove.

New one is called `DMapWithPatchingMove`
This is the one that doesn't store patches on the "to" side.
Now that it doesn't contain patches, it doesn't need so many params.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants