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

insert builder supports values from iterator #739

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

brahmlower
Copy link
Contributor

@brahmlower brahmlower commented Jan 14, 2024

PR Info

Hey again! 👋

The InsertStatement builder doesn't seem to support adding a dynamic number of items without breaking the functional pattern exhibited throughout the docs. I went ahead and added a method for it since it was pretty simple and was a nice quality of life feature.

Though I've got a PR here, there are a few points that I'd like to ask about:

  • I'm not sure I've chosen a good name for the function. If you have any suggestions or preferences for a better name, please let me know and I can update the PR.

  • The method I've added wraps .values_panic, but I've not added an equivalent method for wrapping .values. I wasn't sure what behavior would be expected of such a function, so I figured I should ask first.
    The two options I see are:

    • proceeds on Ok and early exits on first Err
      pub fn values_from<I>(&mut self, values_iter: impl IntoIterator<Item = I>) -> Result<&mut Self>
      where
          I: IntoIterator<Item = SimpleExpr>,
      {
    • continues iterating when .values returns an Err, but collects the failing items and their errors so that all failures can be returned. I thought briefly about what this interface might look like but I'm not really sure (not sure if that means it's just a bad idea? 😅 )

New Features

  • Added the values_from_panic method to InsertStatement for adding multiple rows at once from something that implements IntoIterator.
    let rows = vec![
        [2.1345.into(), "24B".into()],
        [5.15.into(), "12A".into()],
    ];
    
    let query = Query::insert()
        .into_table(Glyph::Table)
        .columns([Glyph::Aspect, Glyph::Image])
        .values_from_panic(rows)
        .to_owned();

Bug Fixes

  • none

Breaking Changes

  • none

Changes

  • none

@brahmlower
Copy link
Contributor Author

Didn't realize this had had a failing test- got that fixed up 👍

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.

Thankyou

@tyt2y3 tyt2y3 merged commit 9f8315f into SeaQL:master Jan 29, 2024
20 checks passed
@tyt2y3
Copy link
Member

tyt2y3 commented Jan 29, 2024

This is the last update before the next major release haha

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