Feature/pure postgres #2375
Feature/pure postgres #2375
Conversation
martell
force-pushed
the
feature/pure_postgres
branch
from
207414b
to
fa687d4
Compare
|
As already stated in in #2257 we currently do not have the bandwidth to work on this change, therefore at least I won't look into this at least till the diesel 2.0 release. Generally speaking it should be possible to extract the |
|
Hey, I totally understand. I was just trying to update the work you put into this up to master. |
|
@martell I really appreciate the work you've already done in various PR's here, so a big THANKS for that 👍. Getting the github CI branch up do date and working would be great. |
|
I'd be glad to help out where ever I can, though it might be intermittent based on work life schedule, and I might only be able to give feedback on some things for lack of context :) |
|
I've sent you an invitation. Beside of that to clarify a few things:
It's totally fine to have a real life. Just drop me a message as soon as you run out of time for this and I will remove you from the corresponding team again.
As we have already written in other places (especially #1186) it's totally fine not to understand everything. In fact you are encouraged to ask questions whenever something is unclear, also things that are not directly related to the code/PR you are reviewing. |
martell
force-pushed
the
feature/pure_postgres
branch
from
2883fa6
to
2b6bd11
Compare
| use postgresql::error::SqlState; | ||
|
|
||
| if let Some(code) = e.code() { | ||
| let kind = if *code == SqlState::UNIQUE_VIOLATION { |
There was a problem hiding this comment.
should probably implement the From trait in a different file for these
I added the missing mappings
There was a problem hiding this comment.
I've put everything in one file back then, because that made development easier. If you find a good location for this, that would be great.
| fn establish(database_url: &str) -> ConnectionResult<Self> { | ||
| use postgresql::tls::NoTls; | ||
|
|
||
| let client = postgresql::Client::connect(database_url, NoTls) |
There was a problem hiding this comment.
How do we handle tls vs no tls, a feature flag ?
There was a problem hiding this comment.
For now I would assume NoTLS for the generic establish method. For using the Tls support from the postgres crate (or any other connection specific stuff from there) there is a TryFrom<postgres::Client> impl somewhere, so that you can construct a customized connection on your own.
| #[allow(missing_debug_implementations)] | ||
| pub struct PgConnection { | ||
| pub(crate) raw_connection: RawConnection, | ||
| conn: RefCell<postgresql::Client>, |
There was a problem hiding this comment.
This creates a large_enum_variant which makes clippy unhappy.
Suggestions ?
There was a problem hiding this comment.
Just wrapping the inner connection into a box should make clippy happy here.
|
|
It's not clear why this should even happen. If you are sure that's a permanent issue and not a temporary CI issue, it may be worth reporting that with corresponding reproducing code to the upstream postgres crate. |
martell
force-pushed
the
feature/pure_postgres
branch
from
2b6bd11
to
e41c3af
Compare
|
Closed as this is currently not planed as part of diesel. I encourage interested users to publish such a connection implementation as third party crate for now. After such a crate reaches a stable state we can talk about upstreaming parts of that work into diesel. |
Just trying to trigger ci when updating #2257 to master