Skip to content

Delegate rustqlite connection persister to transaction persister #1863

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

casey
Copy link

@casey casey commented Mar 1, 2025

This is a small refactor that hopefully improves readability slightly.

Currently, the rusqlite connection persister and transaction persister are entirely different implementations, even though the connection persister just creates a new transaction, does what the transaction persister does, and then commits.

This makes the relationship a little clearly by explicitly delegating the connection persister implementation to the transaction persister implementation, so the reader doesn't need to manually compare the two implementations to see how they're related.

@evanlinjin
Copy link
Member

Thanks for the PR! I would argue that the original was clearer because it more directly expressed what was happening, instead of having the reader go through one more layer of methods.

@casey
Copy link
Author

casey commented Mar 2, 2025

I can see that. It's a trade off: Either you see what it's doing, or you see that it's the same as the other persister. In my opinion, knowing that it's the same as the other persister is more valuable.

@notmandatory notmandatory moved this to Discussion in BDK Chain Mar 4, 2025
@notmandatory notmandatory moved this from Discussion to Needs Review in BDK Chain Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

3 participants