Improve edit api

[czocher]

This PR implements my take on #86. The edit api now takes ownership of the migration it edits.

Closes #86

[cljoly]

Thanks for tackling this!

I would like to explore other options before we consider introducing take_mut (or similar crates) as a dependency though (reasoning in inline comments).

[cljoly]

I think we can do something like this:

diff --git a/rusqlite_migration/src/builder.rs b/rusqlite_migration/src/builder.rs
index 8ecfcf6..28ccef0 100644
--- a/rusqlite_migration/src/builder.rs
+++ b/rusqlite_migration/src/builder.rs
@@ -1,4 +1,4 @@
-use std::iter::FromIterator;
+use std::{iter::FromIterator, mem};
 
 use include_dir::Dir;
 
@@ -10,6 +10,9 @@ pub struct MigrationsBuilder<'u> {
     migrations: Vec<M<'u>>,
 }
 
+// Placeholder migration used to replace and edit
+const PLACEHOLDER_M: M<'static> = M::up("");
+
 impl<'u> MigrationsBuilder<'u> {
     /// Creates a set of migrations from a given directory by scanning subdirectories with a specified name pattern.
     /// The migrations are loaded and stored in the binary.
@@ -42,14 +45,19 @@ pub fn from_directory(dir: &'static Dir<'static>) -> Result<Self> {
     ///
     /// Panics if no migration with the `id` provided exists.
     #[must_use]
-    pub fn edit(mut self, id: usize, f: impl Fn(&mut M)) -> Self {
+    pub fn edit(mut self, id: usize, f: impl Fn(M)) -> Self {
         if id < 1 {
             panic!("id cannot be equal to 0");
         }
-        f(self
-            .migrations
-            .get_mut(id - 1)
-            .expect("No migration with the given index"));
+
+        let migration_to_edit = mem::replace(
+            self.migrations
+                .get_mut(id - 1)
+                .expect("No migration with the given index"),
+            PLACEHOLDER_M.clone(),
+        );
+
+        f(migration_to_edit);
         self
     }

What do you think @czocher ?

[czocher]

What do you think @czocher ?

We still need to copy the PLACEHOLDER which is not perfect. Maybe we can implement Default for M, which would return M("") and use https://doc.rust-lang.org/std/mem/fn.take.html? I'll implement it to show you how it would look like.

[cljoly]

I see two issued with implementing Default:

1. Users of the library will be tempted to use it and maybe that’s inadvisable
2. I’m not sure that calling M::default is cheaper than cloning a constant made up of just pointers & other small values.

If we want to avoid the cloning, we could keep a Vec<Option> inside MigrationsBuilder and then use the take method of Option. Then we put f(M) back in place.

[czocher]

If we want to avoid the cloning, we could keep a Vec inside MigrationsBuilder and then use the take method of Option. Then we put f(M) back in place.

That's the best case in my opinion. I'll implement it and check how it'll look like.

[cljoly]

I'll implement it and check how it'll look like.

I suspect it’ll be easier if we merge #92 first. I’m happy to approve that other PR once we discuss the points raised there.

[czocher]

I'll implement it and check how it'll look like.

I suspect it’ll be easier if we merge #92 first. I’m happy to approve that other PR once we discuss the points raised there.

Let's do that first, yes.