Electron100 / butane

An ORM for Rust with a focus on simplicity and on writing Rust, not SQL
Apache License 2.0
91 stars 13 forks source link

Reexposes r2d2::ManageConnection in r2 #35

Closed joaommartins closed 1 year ago

joaommartins commented 1 year ago

This change is done to prevent having to add r2d2 dependency directly to Cargo.toml.

Previous usage:

# Cargo.toml

[dependencies]
butane = { version = "0.5.1", features = ["default", "r2d2"] }
r2d2 = "0.8.10"
use butane::db::ConnectionManager;
use r2d2::ManageConnection;

New usage:

# Cargo.toml

[dependencies]
butane = { version = "0.5.1", features = ["default", "r2d2"] }
use butane::db::{ConnectionManager, ManageConnection};
Electron100 commented 1 year ago

Hmm, I'm not sure I'm in support of this. The goal of the r2d2 feature is "if you want to use r2d2 then butane integrates with it". In general I'm not a big fan of re-exports as it hides the real origin of the symbol, and just to saving a line in Cargo.toml and a use line doesn't seem like a major win.

Open to being convinced further though.

I do realize that ConnectionManager was already exported. That was likely a typo/error on my part.

jayvdb commented 1 year ago

Would it be more palatable if r2d2::ManageConnection was re-exported in butane::db::r2? That way, it isn't dependent on the r2d2 feature whether it is in the butane::db or not.

Electron100 commented 1 year ago

Yeah, putting re-exports in their own module would definitely be a cleaner way to go, especially if we add support for other connection managers in the future (e.g. once we get async we'll probably want deadpool, and maybe others). I'm still not sure I really see the point of re-exporting, but if it's something you both think would be helpful I don't really mind, if it happens in its own module.

joaommartins commented 1 year ago

I still would prefer to re-expose the trait, since this prevents version issues if the Cargo.toml version used to import the trait happens to be different to the version that butane implements.

I've updated it now, with the trait re-exposing happening in the r2 module. For consistency sake, it would make sense to me to move the r2 ConnectionManager struct exposing to the r2 mod as well, since there may be other connection managers at a later date. However, I did not move this now, since this will break backward compatibility and thus needs a deprecation warning to be issues for a few versions at least.