-
Notifications
You must be signed in to change notification settings - Fork 169
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 limitMaybe_, offsetMaybe_ + tests #633
base: master
Are you sure you want to change the base?
Conversation
=> Maybe Integer -> Q be db (QNested s) a -> Q be db s (WithRewrittenThread (QNested s) s a) | ||
limitMaybe_ (Just limit') (Q q) = | ||
Q (liftF (QLimit limit' q (rewriteThread (Proxy @s)))) | ||
limitMaybe_ Nothing (Q q) = Q (unsafeCoerce q) |
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 you should be able to use rewriteThread
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.
Sorry, could you give a hint on how that would look? I haven't grokked this stuff yet...
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.
@tathougies I'm going through old PRs in and was blocked on this question -- could you clarify?
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, so the type doesn't match I believe because (QNested s)
doesnt unify with
WithRewrittenThread (QNested s) s. The latter unwraps one level of
QNested`, which is what the rewriteThread function is fro. I believe if you do.
rewriteThread (Proxy @s) q
instead of the unsafeCoerce q
, that should work.
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 tried some stuff like that and wasn't able to get it to work:
Building library for beam-core-0.9.2.1..
[28 of 32] Compiling Database.Beam.Query.Combinators
/home/tom/tools/beam/beam-core/Database/Beam/Query/Combinators.hs:347:32: error:
• Couldn't match type: WithRewrittenThread
s0 s (QM be db (QNested s) a)
with: Control.Monad.Free.Church.F
(QF be db s) (WithRewrittenThread (QNested s) s a)
Expected: QM be db s (WithRewrittenThread (QNested s) s a)
Actual: WithRewrittenThread s0 s (QM be db (QNested s) a)
The type variable ‘s0’ is ambiguous
• In the first argument of ‘Q’, namely
‘(rewriteThread (Proxy @s) q)’
In the expression: Q (rewriteThread (Proxy @s) q)
In an equation for ‘limitMaybe_’:
limitMaybe_ Nothing (Q q) = Q (rewriteThread (Proxy @s) q)
• Relevant bindings include
q :: QM be db (QNested s) a
(bound at Database/Beam/Query/Combinators.hs:347:24)
limitMaybe_ :: Maybe Integer
-> Q be db (QNested s) a
-> Q be db s (WithRewrittenThread (QNested s) s a)
(bound at Database/Beam/Query/Combinators.hs:345:1)
|
347 | limitMaybe_ Nothing (Q q) = Q (rewriteThread (Proxy @s) q)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
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.
@tathougies pinging again, would love to get this merged
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.
@tathougies another ping, please help! or maybe we could just merge as-is?
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.
@tathougies here is the ping for 2024 :)
=> Maybe Integer -> Q be db (QNested s) a -> Q be db s (WithRewrittenThread (QNested s) s a) | ||
offsetMaybe_ (Just offset') (Q q) = | ||
Q (liftF (QOffset offset' q (rewriteThread (Proxy @s)))) | ||
offsetMaybe_ Nothing (Q q) = Q (unsafeCoerce q) |
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.
Same here.
Heard there was a major release cycle coming :).
This would resolve #491.
Note: I used an
unsafeCoerce
to implement these. I think it's fine and the tests pass, but maybe there's a nicer way to wrangle the types here?