Repository navigation
Use resourcet-pool - #1163
Use resourcet-pool#1163
Conversation
parsonsmatt
left a comment
parsonsmatt
left a comment
There was a problem hiding this comment.
This MR isn't entirely compatible - resourcet-pool depends on newer version of resource-pool and resourcet than persistent does. I'm curious if persistent is wrong here, or if you can relax the bounds in resourcet-pool.
Overall I like it - factoring out functionality like this is generally a good thing.
| - ./persistent-qq | ||
|
|
||
| extra-deps: | ||
| - resourcet-pool-0.1.0.0 |
There was a problem hiding this comment.
We need this in stackage if we're going to depend on it and continuing offering persistent as a Stackage library.
There was a problem hiding this comment.
I just pushed it to Hackage. I'll put in on Stackage later today. I won't merge this PR until that's done
There was a problem hiding this comment.
@parsonsmatt Ok I'm adding it here: commercialhaskell/stackage#5758
When it gets merged and the library is added to nightly, what should I change in this PR? If I just remove extra-deps from stack.yaml here, stack build will no longer work because stack.yaml uses lts-14.27, right?
There was a problem hiding this comment.
We'll need to keep it in extra-deps, but any nightly checks shouldn't need it starting soon.
| _ -> P.putResource localPool res | ||
|
|
||
| return $ fst <$> mkAcquireType (P.takeResource pool) freeConn | ||
| unsafeAcquireSqlConnFromPool = MonadReader.asks poolToAcquire |
There was a problem hiding this comment.
The implementation of poolToAcquire appears to be copied from this function. This function has an unsafe prefix, but the function you're copying doesn't have any mention of unsafety in any way.
Looking at the docs, I think the "unsafety" might be coming from the fact that this Acquire doesn't start/commit a transaction. So it may be no real concern.
There was a problem hiding this comment.
Yeah, I basically just factored out this function. I agree, it seems like the unsafety is persistent-wise, not resource-pool-wise.
What do you mean? It seems like persistent has , resource-pool >= 0.2.3
, resourcet >= 1.1.10whereas , resource-pool >=0.2.1.0 && <0.3
, resourcet >=1.1.2 && <2 |
Oh man. I think I was confusing which library I was looking at 😂 Good catch! |
|
@parsonsmatt This was added to nightly: commercialhaskell/stackage#5758. Is there anything else blocking this PR? |
|
I fixed CI on master, try merging that in |
|
@parsonsmatt seems to still be failing |
|
I don't think you merged |
|
✨ This is an old work account. Please reference @brandonchinn178 for all future communication ✨ @parsonsmatt sorry about that! should be good to go now |
|
Yeah, looks good! Just needs the CHANGELOG and version bumps for a release. Should be a patch release since there's no change to the behavior of the code. |
|
Sounds good! How's that? |
This reverts commit f82154f.
3 participants
Before submitting your PR, check that you've:
@sincedeclarations to the HaddockAfter submitting your PR:
(unreleased)on the Changelog