Skip to content

Conversation

@fpringle
Copy link

Tiny PR adding an Ord instance to TableIdentifier so that I can use it as a key in a Map in effectful-opaleye.

I understand there's an argument to be made against the Eq, since there's a context in which schema.table and table might be considered equal, despite not comparing equal as Haskell values. If this is an issue, I'd be happy to swap the instances out for a Hashable instance (the hashable package is already a transitive dependency, so it wouldn't add to the footprint).

@tomjaguarpaw
Copy link
Owner

Hmm, I'm a bit dubious about using this internal datatype in another library. Is there some public data type you could use instead?

@fpringle
Copy link
Author

Not in Opaleye, but now you mention it I do notice that postgresql-simple has QualifiedIdentifier which seems to be isomorphic. I'll still need to depend on TableIdentifier, which I get from Inserts, Deletes and Updates in order to keep a running tally of how many DB operations are being dispatched.

@tomjaguarpaw
Copy link
Owner

I see. Table could actually incorporate a type like that. It doesn't have to be factored the way that it currently is.

For now can you just use your own custom type as the Map key and marshall between it and TableIdentifier when necessary? If you use that for six months and you're sure it's the right type for the job come back and I'll think again about what we can expose from the public API to do the job.

@fpringle
Copy link
Author

I'd imagine you could replce TableIdentifier with QualifiedIdentifier pretty easily, since they're identical (except field names), and internal.
I'll do as you suggested, except I'll just use QualifiedIdentifier as the map key.
No matter what type I use, I'll need to be able to get some kind of table identifier from the Insert, Delete and Update types.

@fpringle
Copy link
Author

In case you're curious, this PR contains the code that uses QualifiedIdentifier as a map key to count DB operations.

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

Successfully merging this pull request may close these issues.

2 participants