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

Converted ToSql usage to impl IntoIterator #446

Closed
wants to merge 1 commit into from
Closed

Converted ToSql usage to impl IntoIterator #446

wants to merge 1 commit into from

Conversation

kaibyao
Copy link
Contributor

@kaibyao kaibyao commented Jun 21, 2019

Addresses #265 and #445. This is about as far as I could get with me being new to Rust.

There's still issues:

  • I'm not happy that empty params (&[]) has to be explicity cast to &[some type that implements ToSql (like i32)]. Example: &[] as &[i32]. I'd prefer being able to use the existing method of just sending an empty &[], as that would cause no breakage with existing usage.
  • Also, test test_bpchar_params fails because I don't know how to work with None::<&'static str>.
  • I haven't figured out the syntax for sending parameters that contain different types.

@sfackler, could you take a look at this and see how we might be able to fix these things?

@sfackler
Copy link
Owner

The type inference regressions in things like &[] is pretty prohibitive on making this change IMO. One option would be to keep the current methods and add new ones that take iterators.

@kaibyao
Copy link
Contributor Author

kaibyao commented Jun 23, 2019

I agree; closing this for now as it's not ideal.

@kaibyao kaibyao closed this Jun 23, 2019
@norcalli
Copy link

norcalli commented Jul 2, 2019

The solution for the &[] empty case is typically to make a utility function which has a concrete type such as fn empty() -> &'static [u64; 0] which users can call as .execute(query, empty()). Considering 0.16 is still in RC mode, would it be something that could go in that?

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.

3 participants