-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
implement array_dims #4190
implement array_dims #4190
Conversation
This reverts commit 27bc1d6.
I can't fix the doc test i've written. Help would be appreciated |
a1a20cc
to
f7a7f62
Compare
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.
Thanks for open the PR!
I let some comment about the errors. Could you also add a test with a None input to check it behavior? Thanks!
/// let dims = diesel::select(array_dims::<Array<_>,_,_>(vec![vec![1,2,3],vec![4,5,6]])) | ||
/// .get_result::<String>(connection).unwrap(); | ||
/// assert!(String::from("[1:2][1:3]").eq(&dims)); |
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.
The crazy error here say that diesel didn't know how to transform Vec<Vec<_>>
to Array<_>
because Diesel does not support multidimensional arrays yet. So you can remove this test for now.
/// # use diesel::sql_types::{Nullable,Array,Integer}; | ||
/// # let connection = &mut establish_connection(); | ||
/// | ||
/// let dims = diesel::select(array_dims::<Array<_>,_,_>(vec![1,2])) |
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.
/// let dims = diesel::select(array_dims::<Array<_>,_,_>(vec![1,2])) | |
/// let dims = diesel::select(array_dims::<Array<Integer>,_,_>(vec![1,2])) |
The error says that rust don't know with implementation use, this i32
could be Int4
, Int8
or something else, to fix you need to specify the type here. The array_append
doesn't need this because it Array inner type is the same of the second argument, and the second argument is specified
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.
Thanks for the help!
Thanks for working on this. The PR looks now good 👍 |
02c080c
to
f091f57
Compare
…to feat/add/array_dims
Head branch was pushed to by a user without write access
@weiznich I also resolved the conflicts and merged the branches, is that a problem? |
@valkrypton I's fine, I just tend to resolve conflicts on accepted merge conflicts on my own to not burden contributors with the merge order. |
@weiznich I would love to contribute more to the repo, how do you recommend I start learning about Diesel internals? |
@valkrypton Well the best way to learn about internals is to contribute and ask questions as soon as you hit a problem. It is also usful to join the reviewers teams (see #1186) and read code written by others. That is often an excellent place to learn more about how others approach certain problems and you often get explanations there on how certain things are implemented. |
Added support for
array_dims
postgres function under #4153This is my first PR ever so I would love some poitners!