-
-
Notifications
You must be signed in to change notification settings - Fork 178
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
issues-336 Third try impl postgres-array #467
issues-336 Third try impl postgres-array #467
Conversation
@tyt2y3 @billy1624 can you look next my idea?) |
For me any solution that cant be extended to support custom types is a no go. That is why the trait based solution is used by all other libraries. |
I try to implement trait solution, but got a lot of problems. I still don't understand how to solve them all... |
@@ -17,7 +17,7 @@ rust-version = "1.60" | |||
[lib] | |||
|
|||
[dependencies] | |||
sea-query = { version = "^0", path = ".." } | |||
sea-query = { version = "^0", path = "..", features = ["thread-safe"] } |
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.
This makes sense
This looks good, especially as an intermediate step. The crate architecture has been revamped in 0.27 that is waiting for a release. What's important now is to roll out the next iteration of SeaORM. And maybe I can give a try on |
835707e
to
ac92bd1
Compare
@tyt2y3 @billy1624 @Sytten hello! I have done with some part of this PR.
TODO:
|
Great work! I think now I have an idea as to how ValueTrait would work with SQLx, I am willing to give a try after this settled and we released 0.27 |
This seems to be very nearly done? |
@ikrivosheev check this 79625e0 , not sure if it really works at downstream, but it compiles |
I want to add support for |
…_v3' into feature/issues-336_array_support_v3
Or later... @billy1624 @tyt2y3 I updated the description. Can you review the PR? |
sqlx = { version = "^0.6.1", optional = true } | ||
serde_json = { version = "^1", optional = true } | ||
chrono = { version = "^0.4", default-features = false, features = ["clock"], optional = true } |
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.
can we pub all these from a special export
module in sea-query, then we don't have to specify the dependency twice?
I can imagine the two Cargo.toml might go out of sync in the future.
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.
We want to make our third-party libraries as part of public interface?
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.
Good point
Cool enough. @billy1624 thoughts? |
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.
Wow!! That's smart!
src/value.rs
Outdated
fn except(v: Value, msg: &str) -> Self { | ||
Self::try_from(v).expect(msg) | ||
} |
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.
Would it be better to rename it as expect
? Since it's a wrapper method of try_from
then expect
.
This also align with the API of... few lines above it
fn unwrap(v: Value) -> Self {
Self::try_from(v).unwrap()
}
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.
Fix!
src/value.rs
Outdated
pub fn except<T>(self, msg: &str) -> T | ||
where | ||
T: ValueType, | ||
{ | ||
T::except(self, msg) | ||
} |
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.
Same here :)
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.
Fix!
ArrayType::Bool => { | ||
let value: Option<Vec<bool>> = | ||
Value::Array(ty, v).except("Invalid type for array value"); | ||
args.add(value) | ||
} |
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 think ordinary user won't see this message (unless someone intentionally construct a vector of boxed value with different value variants in it) but the error message itself can be more specific.
Value::Array(ty, v).except("This Value::Array should consist of Value::Bool");
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.
Done
src/value.rs
Outdated
@@ -222,7 +323,7 @@ macro_rules! type_to_value { | |||
} | |||
|
|||
macro_rules! type_to_box_value { | |||
( $type: ty, $name: ident, $col_type: expr ) => { | |||
( $type: ty, $name: ident, $col_type: expr, $array_type: expr ) => { |
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.
Nitpick: a trivial refactoring
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.
Thank you!
Looks good to me except comments above :D |
Reuse $name ident in type_to_box_value macro for ArrayType variant
@billy1624 thank you for review! I corrected all comments |
src/value.rs
Outdated
@@ -185,6 +274,13 @@ impl Value { | |||
{ | |||
T::unwrap(self) | |||
} | |||
|
|||
pub fn except<T>(self, msg: &str) -> T |
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.
Are we keeping this as except
?
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.
Thank you! Fix
One last thing, can we also update the sqlx-postgres example (with several Array types) and make it part of our CI? |
PR Info
Other PR:
This is the third try to impl postgres-array in SeaQuery. Here I store type information into
Value
and then try to convertVec<Value>
intoVec<T>
. This look safe.Adds
sea-query-binder
enum ArrayType
with all possible typesValue::except
methodBreaking Changes
ColumnType::Array
storeBox<ColumnType>
instead stringValue::Array
store addition field:ArrayType
, where store type of array elementsChanges
Iden::to_string