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

connection.query_one/all with update and delete are a bit annoying to use #29

Closed
sgrif opened this issue Nov 29, 2015 · 7 comments
Closed

Comments

@sgrif
Copy link
Member

sgrif commented Nov 29, 2015

It used to be the case that everything had to go through Connection#query_all and Connection#query_one. I found it was annoying, and added too many parenthesis, which is why load and first were added. (connection.query_all(users.filter(name.eq("Sean"))) vs users.filter(name.eq("Sean")).load(&connection).

I want to give the same treatment to these (technically load actually works fine, but I think that update(users.filter(id.eq(1))).set(name.eq("Sean")).load(&connection) reads really strangely). We should add some new methods (the names are up for debate):

  • run(&connection): We often know that we will return exactly one row. The should return QueryResult<T>, and will basically be load(&connection).map(|mut r| r.nth(0).unwrap()).
  • run_all(&connection): Alias for load. Probably a better name for this, but I want it to imply that we're doing a command that happens to return.
  • execute(&connection): Alias for Connection#execute_returning_count
@sgrif
Copy link
Member Author

sgrif commented Nov 29, 2015

I think once we add these methods, it'll also be fine to have first do limit(1) automatically.

sgrif added a commit that referenced this issue Nov 29, 2015
Writing this got me thinking about some API changes for consistency.
See #29 and #30.
@sgrif sgrif added the accepted label Nov 29, 2015
@sgrif sgrif modified the milestones: 0.3, 0.2 Nov 30, 2015
@mfpiccolo
Copy link
Contributor

I am a little confused about where the API is going to end up after this and #30. The pattern that I am hoping it ends up is building the command and calling an execute/run/run_all/load function on that command for all database interactions. Something like:

users::table.load(&conn)                                           // All
users.filter(name.eq(name)).load(&conn)                            // filter
update(users.filter(id.eq(id))).set(name.eq(new_name)).run(&conn)  // update
insert(&users::table, &new_user).run(&conn)                        // create
insert(&users::table, [&new_user]).run_all(&conn)                  // batch create
delete(users.filter(id.eq(user.id))).execute(&conn)                // delete

Either that or calling all of them off of the connection themselves.

@sgrif
Copy link
Member Author

sgrif commented Nov 30, 2015

@mfpiccolo You just described exactly what I want (Although I'm still unsure if run_all is a good name)

@sgrif
Copy link
Member Author

sgrif commented Nov 30, 2015

also I'm leaning towards insert(&new_user).into(&users::table).run(&conn) over what's listed in the posts.

@mfpiccolo
Copy link
Contributor

Yeah. That is more semantic. Will delete also support?:

delete(&existing_user).from(&users::table).execute(&conn)

@sgrif
Copy link
Member Author

sgrif commented Nov 30, 2015

It'll likely just be delete(existing_user).execute(&conn). If there's a primary key, we know what table to use from codegen.

@mfpiccolo
Copy link
Contributor

👍

@sgrif sgrif closed this as completed in 3cb4ad2 Nov 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants