/diesel

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

Explore merging `r2d2-diesel` into Diesel itself #1348

Closed
sgrif opened this Issue Dec 3, 2017 · 1 comment

Comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
  1.1
2 participants
@sgrif @theduke
@sgrif
Member

sgrif commented Dec 3, 2017

This was brought up in the impl period working group channel. The more I think about it, the more I like this idea. I started thinking about this again recently when I was looking at the "database" keyword on crates.io. As of the time of this issue, r2d2 shows up before any library that people there are likely actually looking for (diesel, rusqlite, or postgres). It makes sense for that to be the case. Anybody using Diesel (except for sqlite users) or rust-postgres are almost certainly going to rely on r2d2. It'd make sense for them to remove the database keyword, but at the same time, neither of those libraries mentions r2d2 in their docs, so I'm not sure where people would find it.

The point of all this is that I'm warming up to the idea of merging r2d2-diesel into Diesel itself. I think it still needs some exploration though. I'd like to see how much of the code from https://gitter.im/rust-impl-period/WG-libs-diesel?at=5a1d24628b3a9e2c0c27d69f actually goes away if we moved it into Diesel (just having aliases in Diesel is not good enough IMO, we need to abstract over some of those types to the extent that users don't have to care about them).

If nothing else, having impl<T: Connection> Connection for PooledConnection<T> will help a ton, and remove the need for explicit derefs which can be annoying and confusing for newcomers.

I've placed this issue on the 1.1 milestone, but again, this issue is for "exploring" this option, not for sure making it happen.

@sgrif sgrif added this to the 1.1 milestone Dec 3, 2017

sgrif added a commit that referenced this issue Jan 8, 2018

@sgrif
Merge r2d2-diesel into Diesel itself 
The main benefit of doing this is that we can implement `Connection` for
`PooledConnection`, removing the requirement to do `&*conn` in your
outermost functions. Additionally, we can just re-export r2d2 from
within Diesel itself, so we can hopefully get fewer bug reports saying
"Hey why does it say PgConnection doesn't implement Connection". Instead
people can just `use diesel::r2d2::Pool` instead of explicitly depending
on r2d2.

The code and the tests are both stripped straight from r2d2-diesel.
(Yeah, the tests are a bit... opaque. They come from the initial commit
of that repo, with the commit message "The tests have been pretty much
ripped from r2d2-postgres, as I really don't know what else to do to
test this". So blame r2d2-postgres I guess)

The only changes between this code and what is in r2d2-diesel 1.0 is the
`Connection` impl. This impl has to be a little bit funky. I decided to
just restrict to `AnsiTransactionManager` since that's what all three
backends use at the moment. If we wanted to support anything that
implemented `diesel::Connection`, we would need to add the constraint
`C::TransactionManager: TransactionManager<Self>`. However,
`TransactionManager` has the bound that it's parameter be `Connection`,
so we just recurse infinitely. Maybe on day the language will be smarter
about this. Until then, we just have to do this weird hack.

Fixes #1348.
8dc9093

sgrif added a commit that referenced this issue Jan 8, 2018

@sgrif
Merge r2d2-diesel into Diesel itself 
The main benefit of doing this is that we can implement `Connection` for
`PooledConnection`, removing the requirement to do `&*conn` in your
outermost functions. Additionally, we can just re-export r2d2 from
within Diesel itself, so we can hopefully get fewer bug reports saying
"Hey why does it say PgConnection doesn't implement Connection". Instead
people can just `use diesel::r2d2::Pool` instead of explicitly depending
on r2d2.

The code and the tests are both stripped straight from r2d2-diesel.
(Yeah, the tests are a bit... opaque. They come from the initial commit
of that repo, with the commit message "The tests have been pretty much
ripped from r2d2-postgres, as I really don't know what else to do to
test this". So blame r2d2-postgres I guess)

The only changes between this code and what is in r2d2-diesel 1.0 is the
`Connection` impl. This impl has to be a little bit funky. I decided to
just restrict to `AnsiTransactionManager` since that's what all three
backends use at the moment. If we wanted to support anything that
implemented `diesel::Connection`, we would need to add the constraint
`C::TransactionManager: TransactionManager<Self>`. However,
`TransactionManager` has the bound that it's parameter be `Connection`,
so we just recurse infinitely. Maybe on day the language will be smarter
about this. Until then, we just have to do this weird hack.

Fixes #1348.
89e0866

@sgrif sgrif referenced this issue Jan 8, 2018

Merged

Merge r2d2-diesel into Diesel itself #1466

sgrif added a commit that referenced this issue Jan 8, 2018

@sgrif
Merge r2d2-diesel into Diesel itself 
The main benefit of doing this is that we can implement `Connection` for
`PooledConnection`, removing the requirement to do `&*conn` in your
outermost functions. Additionally, we can just re-export r2d2 from
within Diesel itself, so we can hopefully get fewer bug reports saying
"Hey why does it say PgConnection doesn't implement Connection". Instead
people can just `use diesel::r2d2::Pool` instead of explicitly depending
on r2d2.

The code and the tests are both stripped straight from r2d2-diesel.
(Yeah, the tests are a bit... opaque. They come from the initial commit
of that repo, with the commit message "The tests have been pretty much
ripped from r2d2-postgres, as I really don't know what else to do to
test this". So blame r2d2-postgres I guess)

The only changes between this code and what is in r2d2-diesel 1.0 is the
`Connection` impl. This impl has to be a little bit funky. I decided to
just restrict to `AnsiTransactionManager` since that's what all three
backends use at the moment. If we wanted to support anything that
implemented `diesel::Connection`, we would need to add the constraint
`C::TransactionManager: TransactionManager<Self>`. However,
`TransactionManager` has the bound that it's parameter be `Connection`,
so we just recurse infinitely. Maybe on day the language will be smarter
about this. Until then, we just have to do this weird hack.

Fixes #1348.
0bfe801

sgrif added a commit that referenced this issue Jan 13, 2018

@sgrif
Merge r2d2-diesel into Diesel itself 
The main benefit of doing this is that we can implement `Connection` for
`PooledConnection`, removing the requirement to do `&*conn` in your
outermost functions. Additionally, we can just re-export r2d2 from
within Diesel itself, so we can hopefully get fewer bug reports saying
"Hey why does it say PgConnection doesn't implement Connection". Instead
people can just `use diesel::r2d2::Pool` instead of explicitly depending
on r2d2.

The code and the tests are both stripped straight from r2d2-diesel.
(Yeah, the tests are a bit... opaque. They come from the initial commit
of that repo, with the commit message "The tests have been pretty much
ripped from r2d2-postgres, as I really don't know what else to do to
test this". So blame r2d2-postgres I guess)

The only changes between this code and what is in r2d2-diesel 1.0 is the
`Connection` impl. This impl has to be a little bit funky. I decided to
just restrict to `AnsiTransactionManager` since that's what all three
backends use at the moment. If we wanted to support anything that
implemented `diesel::Connection`, we would need to add the constraint
`C::TransactionManager: TransactionManager<Self>`. However,
`TransactionManager` has the bound that it's parameter be `Connection`,
so we just recurse infinitely. Maybe on day the language will be smarter
about this. Until then, we just have to do this weird hack.

Fixes #1348.
df6d43d

@sgrif sgrif closed this in #1466 Jan 13, 2018

@theduke

This comment has been minimized.

Sign in to view
Contributor

theduke commented Jan 26, 2018

@sgrif thanks for picking up my suggestion and implementing it, it's much appreciated!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment