Skip to content
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

Extract generic traits from postgresql structs #72

Closed
wants to merge 2 commits into from

Conversation

tafia
Copy link

@tafia tafia commented Dec 28, 2015

In order to allow multiple database implementations,
Connection and DbResult have been promoted to Traits instead of structs

A new folder postgresql have been added with the relevant postgresql implementations (PGConnection, PGDbResult and the query builder)

This is a first step toward a generic ORM. Hopefully implementing diesel for new databases will be a matter of adding a new folder + some sql changes.

NOTE: I don't have a postgresql installed on my machine so I couldn't run the tests. I even couldn't compile tests because of linking issues. Still I think this is a good first step ....

In order to allow multiple database implementations,
`Connection` and `DbResult` have been promoted to Traits instead of structs

A new folder *postgresql* have been added with the relevant postgresql
implementation.
@tafia tafia changed the title Extract generic trait from postgresql structs Extract generic traits from postgresql structs Dec 28, 2015
@sgrif
Copy link
Member

sgrif commented Dec 29, 2015

Thanks for the PR. There's some failing tests, but I think I can handle getting them passing. I think it's still a bit premature to try and genericise these interfaces, but this all seems pretty reasonable at first glance. This is a pretty hefty PR, so it'll take me a day or two to get through it.

@tafia
Copy link
Author

tafia commented Dec 29, 2015

Of course I understand.
I didn't really try reasoning about what should be generic or not. My main
focus was to ensure postgresql was in its own module without breaking too
much things.
Take your time.
Thanks
On 29 Dec 2015 09:18, "Sean Griffin" notifications@github.com wrote:

Thanks for the PR. There's some failing tests, but I think I can handle
getting them passing. I think it's still a bit premature to try and
genericise these interfaces, but this all seems pretty reasonable at first
glance. This is a pretty hefty PR, so it'll take me a day or two to get
through it.


Reply to this email directly or view it on GitHub
#72 (comment).

also rename
- `PGConnection` to `PgConnection`
- `PGDbResult` to `PgDbResult`
@tafia
Copy link
Author

tafia commented Dec 30, 2015

Pushed one more commit to relocate postgresql data types and extensions to postgresql directory.
Contrary to Connection/DbResult, the code was already generic. The only changes are use expressions within postgresql folder and lib.rs

EDIT: Also renamed PGConnection/PGDbResult to PgConnection/PgDbResult to align with data types names (and they feel more rustic)

@sgrif
Copy link
Member

sgrif commented Jan 15, 2016

Hey just wanted to update why I haven't given this much attention lately. I've started to give more thought to sqlite 3 support, and there's a couple of hard problems that need to be solved.

  • Our NativeSqlType API only makes sense for PG, but there's not an obvious other place to put it.
  • The way we handle bind parameters during query construction works fine for PG and MySQL's API, but is fundamentally opposed to SQLite's.

Both of these lead me to believe that we very well might end up simply having the traits themselves have different APIs based on a cfg flag. If that is the route that we end up going down, I'm not sure that it actually makes sense to promote some of these to traits (though it might be worth it any way)

In any case, I'm hesitant to do some of the easier changes until we figure out the harder ones.

@sgrif
Copy link
Member

sgrif commented Jan 15, 2016

Note: I wrote up some very long thoughts on this issue overall in #39 (comment)

_marker: PhantomData<(ST, T)>,
}

impl<ST, T> Cursor<ST, T> {
impl<ST, T, R> Cursor<ST, T, R> where
Copy link
Member

Choose a reason for hiding this comment

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

While we may go this route in the future, to limit the scope of this PR, I'd prefer that we did not change this type, and instead had the various methods on Connection return Box<Iterator>

@sgrif
Copy link
Member

sgrif commented Jan 23, 2016

@tafia Thanks for starting the work on this, but there's a lot of feedback that still needs to be addressed, and we're now at the point where this change is actually needed. I'm going to close in favor of #128.

@sgrif sgrif closed this Jan 23, 2016
@tafia
Copy link
Author

tafia commented Jan 24, 2016

No pb!
On 24 Jan 2016 01:18, "Sean Griffin" notifications@github.com wrote:

@tafia https://github.com/tafia Thanks for starting the work on this,
but there's a lot of feedback that still needs to be addressed, and we're
now at the point where this change is actually needed. I'm going to close
in favor of #128 #128.


Reply to this email directly or view it on GitHub
#72 (comment).

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.

None yet

2 participants