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

Runner IntoIterator support #293

Closed
wants to merge 9 commits into from
Closed

Conversation

superstator
Copy link
Contributor

  • adds an impl of IntoIterator for Runner, as a third way to run migrations beyond run() and run_async()
  • moves the connection argument from run() to Runner::new()
  • refactors some migration code to reduce duplication between sync/async implementations and make handling of Target::Fake more consistent

@superstator superstator changed the title Iter Runner IntoIterator support Nov 25, 2023
@jxs
Copy link
Member

jxs commented Dec 2, 2023

Hi again, I am sorry for the delay and thanks for your work with this! I see a problem with this implementation imho, having Runner become Runner<'a, C> heavily limits what you can do with it, basically Runner can't leave the scope where it was created, see here for an example.

Even if it might not mean actual practical limitations as there might not be a use case for a user wanting to move Runner across scopes it might exist in the future, and I feel like limiting this for one function is not worth the tradeoff. I checked if we could take ownership into Connection but the first crate checked, rusqlite doesn't impl Clone for Connection so it's a no go.

The solution I see is what we discussed on #119 (comment), having an iter(&self, &mut C) -> Iter<'a, 'b, R, C> which can impl std::iter::Iterator which also impl's std::iter::IntoIterator. I understand it's not conventional, but I am more comfortable with it than limiting Runner to become Runner<'a, C> wdyt?

Thanks!

@superstator superstator mentioned this pull request Dec 5, 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