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

Add SqliteTypedQueryBuilder for exact type mapping #708

Closed
wants to merge 2 commits into from

Conversation

Perksey
Copy link

@Perksey Perksey commented Sep 25, 2023

PR Info

New Features

Adds customizable type mapping to ensure that the exact type can be used on SQLite if the user needs it.

Bug Fixes

Provides the ability for the user to enable BigUnsigned PRIMARY KEY AUTOINCREMENT.

Breaking Changes

I’ve done my best to avoid any breaking changes. This is done by introducing a SqliteTypedQueryBuilder and implementing a SqliteBuilderVariant trait on both builders to deduplicate the code.

Changes

I was hesitant to add a blanket implementation of sea-query types on T where T is a SqliteBuilderVariant, as this would definitely cause future problems, either here or downstream. Therefore, I’ve implemented a sqlite_impl macro which will repeat the implementation for both SqliteTypedQueryBuilder and SqliteQueryBuilder.

I forgot about the need of PhantomData when I originally proposed this, and this can’t be done in a non-breaking manor without separate types.

@Perksey
Copy link
Author

Perksey commented Sep 25, 2023

This can close #689 but discussion is needed for whether we actually want to special case BIGINT AUTOINCREMENT on the default mapper.

@Perksey
Copy link
Author

Perksey commented Sep 25, 2023

@tyt2y3 ready for review, recommend filtering out whitespace changes

write!(
sql,
"{}",
match column_type {
// ----------------------------------------------------------------------------------------------------
// A NOTE REGRADING SQLITE TYPE AFFINITY: It's great that we try to be very on-point with our mappings,
Copy link
Member

Choose a reason for hiding this comment

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

That's a very throughout comment section haha

Copy link
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

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

It looks pretty good.

My only hope is to have a prettier macro that can do the following:

#[impl_many]
impl T for A, B {
    fn method() {}
}

// which would:
impl T for A { .. }
impl T for B { .. }

that would create no doubt about what the sqlite_impl would do.

https://docs.rs/duplicate/latest/duplicate/attr.duplicate_item.html seems to be a pretty good one, do you have other suggestions?

@Perksey
Copy link
Author

Perksey commented Oct 5, 2023

I couldn't find any macros that would support adding a generic parameter for one of the alternate selves, and attempting to make one did not go well!

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 7, 2023

Alternatively, may be we can implement only a subset of the traits?

@tyt2y3
Copy link
Member

tyt2y3 commented Dec 14, 2023

Sorry for the boomerang, but we've now decided to use feature flags for certain options. I'll add a flag

@tyt2y3 tyt2y3 closed this Dec 14, 2023
tyt2y3 added a commit that referenced this pull request Dec 14, 2023
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.

2 participants