Spar Polysemy: Final Cassandra effects #1806
Conversation
There was a problem hiding this comment.
Two small things:
- Could you run
make add-license(I keep forgetting this myself) - It would be nice to have explicit export lists in
Spar.Sem.*. (Optional; we don't do this consisently, but I think at some point we agreed that we'd like to have that. If you don't do it I might make PR to resolveSpar.DataintoSpar.Sem.*, and do it there. I'm hoping to find some dead code that way. :)
| Sem r a | ||
| assIDStoreToCassandra = | ||
| interpret $ \case | ||
| Store itla t -> do |
There was a problem hiding this comment.
No need to fix this now, but in general this is about the scope size where I start to not like one-letter identifiers.
| @@ -221,37 +225,39 @@ spec = do | |||
| idp2' <- runSpar $ liftSem (IdPEffect.getConfig (idp1 ^. idpId)) | |||
| liftIO $ idp2' `shouldBe` Nothing | |||
|
|
|||
| -- TODO(sandy): This function should be more polymorphic over it's polysemy | |||
| -- constraints than using 'RealInterpretation' in full anger. | |||
There was a problem hiding this comment.
not sure that would be an improvement? it's a local function, and the type is obviously flexible enough for it's use cases.
There was a problem hiding this comment.
The point being that it doesn't need all of RealInterpreter, it really just needs a way to run its constraints in IO. Making this more polymorphic would mean we could use the same function to test pure interpreters too.
|
Random question: when I'm blocked in review, I usually base branches on top of each other, not all of them on |
I just realized that we're not the only people not doing this. :') Never mind, I'll do it in a separate PR. |
This branch is based on #1792 :) |
Mind if we do this in a separate PR? |
isovector
force-pushed
the
last-spar-effects
branch
2 times, most recently
from
63205c4 to
0a8714b
Compare
This PR migrates the rest of Spar off depending directly on Casasndra.
To be rebased onto
developwhen #1792 lands.Checklist
changelog.d.