Remove inappropriate SafeToInsert constraints #73
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pbrisbin
approved these changes
Dec 13, 2023
There was a problem hiding this comment.
The type family stuff is a bit over my head, but I'm glad to see this solved.
I think the main open question causing #70 to stall was if introducing insertKeyed could be done without a major version bump. Seems like you're saying it cannot, but let's do it anyway. I tend to agree.
Comment on lines
+27
to
+32
| , KeySourceTypeM | ||
| , KeyForInsert | ||
| , KeyRequirementForInsert | ||
| , InsertWithPossiblyRequiredKey (..) | ||
| , Required (..) | ||
| , Optional (..) |
There was a problem hiding this comment.
Do users bump into this stuff at all, or is it all encapsulated?
There was a problem hiding this comment.
The only concern is it might show up in type errors, not sure. Otherwise no.
chris-martin
force-pushed
the
safe-to-insert
branch
from
8566747
to
c9edcfb
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Persistent 2.14 introduced the
SafeToInsertclass to flag a record as being usable with theinsert; is a record is not "safe to insert" then it must be inserted withinsertKey.#66 introduced a compatibility class
GraphulaSafeToInsertto allowgraphulato be usable with persistent >=2.14. However, it did so by placing excessiveGraphulaSafeToInsertconstraints.node(which handles cases where the record's key source isSourceDefaultorSourceArbitrary) this is somewhat overconstrained;SafeToInsertshould be necessary when the record's key source isSourceDefault, but it should not be necessary when the key isSourceArbitrary, since we will always be generating a key to use for the insert.nodeKeyed, theSafeToInsertconstraint should not be needed at all, since a key has been provided.This PR fixes those constraints, though it isn't easy to see because some of it is buried under a handful of type-level functions. (I acknowledge that this implementation could probably be made prettier, though it's "good enough for now" IMO.)
MonadGraphulaFrontendnow needs a newinsertKeyedfunction in addition toinsert, because insertions with and without a key now require different calls to Persistent. This is the cause for the major version bump, as it will be a breaking change for any instances of the class.Upgrading to this version of graphula (and adding
SafeToInsertconstraints to a handful of functions) is necessary and sufficient for upgradingmegarepo/backendto the latestpersistent.