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

persistent-sqlite 2.10.2/3 should be deprecated #933

Closed
merijn opened this issue Jul 17, 2019 · 8 comments
Closed

persistent-sqlite 2.10.2/3 should be deprecated #933

merijn opened this issue Jul 17, 2019 · 8 comments

Comments

@merijn
Copy link
Contributor

merijn commented Jul 17, 2019

As mentioned in #932 (comment) the instances for RawSqlite were (unfortunately) broken, because the functionality of liftPersist changed between the original patch and persistent 2.10

I fixed the instances and am adding tests, but I hit a snag with the tests. The tests defined in persistent-test all seem to have a backend ~ BaseBackend backend constraint, so I can't simply rerun those tests with the RawSqlite backend without modifying persistent-test. This isn't a hard problem to fix, but I'm not sure if removing that constraint from the various specsWith definitions will break anything...

@merijn
Copy link
Contributor Author

merijn commented Jul 17, 2019

Ping @parsonsmatt

@merijn
Copy link
Contributor Author

merijn commented Jul 17, 2019

I can easily PR the fix (it's pretty trivial), but I'd like to avoid this sorta "discovery right after release" and add tests...

@parsonsmatt
Copy link
Collaborator

Move fast and break things 😂

Can you describe the change in liftPersist that caused a break?

@parsonsmatt
Copy link
Collaborator

Versions are deprecated on Hackage.

@merijn
Copy link
Contributor Author

merijn commented Jul 17, 2019

It used to only accept "ReaderT SqlBackend IO r" and then it'd use projectBackend to get the SqlBackend from the MonadReader. Now it's polymorphic in backend. The result is that all the instances for RawSqlite are recursive and infinite loop >.>

It's trivial to fix, see the new PR.

I'm just unsure how to reuse the existing tests with RawSqlite, as I said, I can simply remove the constraints from the various tests to allow any backend, but I don't know if that has any unforeseen consequences.

@parsonsmatt
Copy link
Collaborator

honestly i'd love to remove the BaseBackedn type family entirely, so a PR purging that from the tests would be welcome

@merijn
Copy link
Contributor Author

merijn commented Jul 17, 2019

Looks like that'd be rather tricky to implement, a lot of the classes (for example PersistQueryWrite, aka updateWhere and deleteWhere) have PersistRecordBackend constraints on the arguments, which means PersistEntityBackend record ~ BaseBackend backend which is making this whole deal rather painful to remove.

I'm not entirely sure what the impact/ramifications of removing that type equality from the various classes would be.

@merijn
Copy link
Contributor Author

merijn commented Jul 17, 2019

Perhaps the best option is to merge #934, make a new persistent-sqlite release that replaces 2.10.2/2.10.3 with one that actually works and make a new/separate issue for refactoring PersistRecordBackend in such a way that we can generalise the tests, because that change is probably big enough to require a bump to 2.11

parsonsmatt added a commit that referenced this issue Jul 24, 2019
Replace liftPersist, fixes #933
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

No branches or pull requests

2 participants