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

SQLite update DDL creation to the new db enum #1120

Closed
wants to merge 1 commit into from

Conversation

Sildra
Copy link
Contributor

@Sildra Sildra commented Jan 17, 2024

No description provided.

@Sildra Sildra force-pushed the sqlite-ddl branch 2 times, most recently from ea33624 to 261a02f Compare January 17, 2024 09:18
@vadz
Copy link
Member

vadz commented Jan 17, 2024

Sorry, could you please explain what does this actually change?

@vadz vadz added the SQLite label Jan 17, 2024
@Sildra
Copy link
Contributor Author

Sildra commented Jan 17, 2024

It is the PR that resolves #1085

Copy link
Member

@vadz vadz left a comment

Choose a reason for hiding this comment

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

Sorry but the test code is very difficult to understand as written: it has no comments and uses unclear or even misleading names. Could you please simplify it to make it actually understandable?

As for the changes themselves, I'm not sure at all about the date type, but otherwise I think they should do no harm. I'd appreciate opinion of the other SQLite users however.

tests/common-tests.h Outdated Show resolved Hide resolved
tests/common-tests.h Outdated Show resolved Hide resolved
tests/common-tests.h Outdated Show resolved Hide resolved
tests/common-tests.h Outdated Show resolved Hide resolved
tests/common-tests.h Outdated Show resolved Hide resolved
tests/common-tests.h Outdated Show resolved Hide resolved
tests/sqlite3/test-sqlite3.cpp Outdated Show resolved Hide resolved
tests/sqlite3/test-sqlite3.cpp Outdated Show resolved Hide resolved
include/soci/sqlite3/soci-sqlite3.h Outdated Show resolved Hide resolved
include/soci/sqlite3/soci-sqlite3.h Outdated Show resolved Hide resolved
@Sildra Sildra force-pushed the sqlite-ddl branch 4 times, most recently from a4766f1 to 3b9b24f Compare January 18, 2024 00:45
@Sildra
Copy link
Contributor Author

Sildra commented Apr 23, 2024

Hello, any news about improving the table creation with the DDL interface ?

Regards,

Copy link
Member

@vadz vadz left a comment

Choose a reason for hiding this comment

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

Thanks, I think the changes are good, with the exception of not recognizing the previously recognized int2 and int8, but it still looks like the testing code could be simplified — or, if not, at least commented more...

src/backends/sqlite3/statement.cpp Show resolved Hide resolved
src/backends/sqlite3/statement.cpp Show resolved Hide resolved
}

template<typename T>
void test_roundtrip(soci::session &sql, Roundtrip<T>&& tester)
Copy link
Member

Choose a reason for hiding this comment

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

Why is tester passed as r-value reference, it doesn't seem to be consumed by this function? Or is it? A comment would be nice...

FWIW I'd probably just pass db_type (unless it can be deduced from T?) and the value directly here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do this, it will solve the r-value ref.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

db_type could be deduced from T but it would require defining templates for this and it would add a lot of code.

@@ -287,9 +287,89 @@ template<> struct type_conversion<PhonebookEntry3>
ind = i_ok;
}
};
} // namespace soci

template<typename T>
Copy link
Member

Choose a reason for hiding this comment

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

A comment explaining what is this type used for would be nice to have.

@vadz
Copy link
Member

vadz commented Apr 24, 2024

Thanks, merged now with minor stylistic corrections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants