/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

Consider changing `insert(records).into(table)` to `insert_into(table).values(records)` #1167

Closed
sgrif opened this Issue Sep 15, 2017 · 2 comments

Comments

Assignees
No one assigned
Labels
Projects
None yet
Milestone
  1.0
2 participants
@sgrif @samphippen
@sgrif
Member

sgrif commented Sep 15, 2017

There are two main things that got me thinking about this change. The first is this code. It's a papercut, but I don't like the fact that if you get the argument to insert wrong, the message is going to be that you passed the wrong number of arguments to into. Maybe this isn't a big deal (I don't think it's ever come up in gitter), but it's a paper cut I'd like to avoid, and I think it will be easier to run into once #789 is done.

The second is that I've been thinking about an API to represent INSERT INTO table (column_list) SELECT .... The best idea I've seen based on our current API is insert(select_statement).into(column_list), but that feels a little implicit to me. If we changed the insert API, I could see something like insert_into(table).columns(column_list).values(select_statement) or insert_into(table).from_select(column_list, select_statement) being much more reasonable.

@sgrif sgrif added breaking change discussion desired labels Sep 15, 2017

@sgrif sgrif added this to the 1.0 milestone Sep 15, 2017

@sgrif

This comment has been minimized.

Sign in to view
Member

sgrif commented Sep 15, 2017

@diesel-rs/contributors WDYT?
@samphippen

This comment has been minimized.

Sign in to view
Contributor

samphippen commented Sep 15, 2017

I like it.

sgrif added a commit that referenced this issue Sep 21, 2017

@sgrif
Deprecate `insert` in favor of `insert_into` 
This change deprecates inserts in the form `insert(values).into(table)`
in favor of `insert_into(table).values(values)`.

There are several reasons for this change:

- The new API more closely mirrors the SQL it creates.
- The old API conflicted with `Into::into`, meaning that if the argument
  to `insert` was wrong, the error message would be that `into` expected
  0 arguments and got 1.
- Things like `default_values` fit more nicely with this API (especially
  after the upsert changes in #1166 land)
- The API for inserting a select statement with an explicit column list
  will work better with this API

I've replaced all instances of `insert` with the new API, except for the
doctests of the deprecated functions.

(As a reminder in case you're wondering what the 0.99.0 is, the plan is
that the next version will be 1.0. 0.99.0 will be identical to 1.0,
except 1.0 will have no deprecated code)

Fixes #1167.
Loading status checks…
307fdfd

@sgrif sgrif referenced this issue Sep 21, 2017

Merged

Deprecate `insert` in favor of `insert_into` #1182

@sgrif sgrif closed this in #1182 Sep 21, 2017

sgrif added a commit that referenced this issue Sep 21, 2017

@sgrif
Deprecate `insert` in favor of `insert_into` (#1182) 
This change deprecates inserts in the form `insert(values).into(table)`
in favor of `insert_into(table).values(values)`.

There are several reasons for this change:

- The new API more closely mirrors the SQL it creates.
- The old API conflicted with `Into::into`, meaning that if the argument
  to `insert` was wrong, the error message would be that `into` expected
  0 arguments and got 1.
- Things like `default_values` fit more nicely with this API (especially
  after the upsert changes in #1166 land)
- The API for inserting a select statement with an explicit column list
  will work better with this API

I've replaced all instances of `insert` with the new API, except for the
doctests of the deprecated functions.

(As a reminder in case you're wondering what the 0.99.0 is, the plan is
that the next version will be 1.0. 0.99.0 will be identical to 1.0,
except 1.0 will have no deprecated code)

Fixes #1167.
Loading status checks…
73778af
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment