-
-
Notifications
You must be signed in to change notification settings - Fork 316
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
Return underlying 64-bit integer value for last inserted row id #1334
Conversation
For the high-level API: * instead of narrowing the actual return value of `sqlite3_last_insert_rowid()` to `int`, `int64` is now returned. * inserting a range of objects also returns the last insert rowid, or 0 if the range is empty.
@@ -852,11 +852,11 @@ namespace sqlite_orm { | |||
} | |||
|
|||
template<class O, class... Cols> | |||
int insert(const O& o, columns_t<Cols...> cols) { | |||
static_assert(cols.count > 0, "Use insert or replace with 1 argument instead"); | |||
int64 insert(const O& o, columns_t<Cols...> cols) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd leave it like that cause changing return type of public API which exists 6+ years will spawn a lot of warnings
@@ -865,11 +865,11 @@ namespace sqlite_orm { | |||
* @return id of just created object. | |||
*/ | |||
template<class O> | |||
int insert(const O& o) { | |||
int64 insert(const O& o) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd leave it like that cause changing return type of public API which exists 6+ years will spawn a lot of warnings. int
is the most popular type and the first type every developer uses in the first C++ program. And also for 7+ years nobody complained about int
here. Who needs the highest precision just uses insert
with last_insert_rowid
calls together. Now one person decided it is wrong and we are adding a lot of warning to all other projects. This is a bad practice. The better solution is to leave this public API as is and tell that one person what to do instead.
this->assert_mapped_type<O>(); | ||
auto statement = this->prepare(sqlite_orm::insert(std::ref(o), std::move(cols))); | ||
return int(this->execute(statement)); | ||
return this->execute(statement); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that this is legacy but it doesn't mean that all my arguments have no meaning
@@ -907,9 +907,9 @@ namespace sqlite_orm { | |||
* ``` | |||
*/ | |||
template<class... Args> | |||
void insert(Args... args) { | |||
int64 insert(Args... args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is one interesting feature in SQLite called RETURNING
https://www.sqlite.org/lang_returning.html. @trueqbit let's imagine how we are going to implement this. E.g. this query is expected to return void
storage.insert(into<User>(),
columns(&User::id, &User::name),
values(std::make_tuple(1, std::string("Ellie"))));
But this query
auto insertedUser = storage.insert(into<User>(),
columns(&User::id, &User::name),
values(std::make_tuple(1, std::string("Ellie"))),
returning(asterisk());
is expected to return a User
. And this query
auto insertedId = storage.insert(into<User>(),
columns(&User::id, &User::name),
values(std::make_tuple(1, std::string("Ellie"))),
returning(&User::id);
is expected to return inserted id no matter what type id has. And RETURNING
feature will just erase this diff. I know I did not tell you about this feature idea but know I am doing this and looks like I am doing this right in time.
@@ -947,28 +947,28 @@ namespace sqlite_orm { | |||
} | |||
|
|||
template<class It, class Projection = polyfill::identity> | |||
void insert_range(It from, It to, Projection project = {}) { | |||
int64 insert_range(It from, It to, Projection project = {}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes no sense why inserting a range has to return inserted id. I mean who needs to insert let's say 10 objects and then expect to get latest object's id. RETURNING
feature is more appropriate here and once it is added your diff in this function will just removed. So let's just skip changing a lot of public APIs cause one person wants it. It is related to both insert_range
overloads
For the high-level insertion API:
sqlite3_last_insert_rowid()
toint
,int64
is now returned.