Skip to content

Clean-up db sub-crate #22

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

Merged
merged 11 commits into from
Feb 18, 2020
Merged

Conversation

LukeMathWalker
Copy link
Contributor

A little bit of polishing/clean-up of our db-related code:

  • encapsulate diesel's errors into an opaque error type (anyhow::Error), so that:
    • domain sub-crate does not have to list diesel as a dependency (nice side-effect: compilation speed-up 🚀)
    • domain remains truly unaware of the underlying persistence-related technology choice (ORM-agnostic).
  • remove the closure-based mechanism for queries, making it possible to get a pooled connection with a method;
  • use the same DatabaseError struct for all DatabaseError error variants in domain;
  • misc micro-improvements.

colinbankier
colinbankier previously approved these changes Feb 18, 2020
Copy link
Owner

@colinbankier colinbankier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has made me realize that something must have been lost along the way that I missed - the initial point of the Repo and the run method was to "Run the given closure in a way that is safe for blocking IO to the database" - so that it doesn't block the tokio reactor. It used task::spawn_blocking to do this. Did I miss something that this not necessary any more with all the async ecosystem advances?

Even though this may not the a standard way to use diesel, it was something I cared about, however maybe it's confusing in an example app.

@colinbankier colinbankier dismissed their stale review February 18, 2020 14:07

Checking async diesel issue made after review.

@LukeMathWalker
Copy link
Contributor Author

LukeMathWalker commented Feb 18, 2020

With the latest version of the async-std runtime (not yet released), we should be able to achieve the same result without any special handling: https://async.rs/blog/stop-worrying-about-blocking-the-new-async-std-runtime/
The tracking PR is async-rs/async-std#631

tide is not using it yet, but it will get it eventually, hence I think it makes sense to simplify the code and get the speedup when that lands.

@colinbankier
Copy link
Owner

Fair enough - I agree. Let's bring that in when available 👍

@colinbankier colinbankier merged commit 16c1842 into colinbankier:master Feb 18, 2020
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