make `generate_managed` always cleanup the container

[dbofmmbt]

I stumbled with #146 while experimenting with cornucopia, so I thought it would be okay to propose a fix.

Fixes #146

[LouisGariepy]

Thanks for your contribution :smile:

The reason we don't always clean up is to make it possible to inspect the database after a codegen failure. Obviously this is pretty niche, and I agree that cleanup-on-failure should be the default, but this PR would make it basically impossible to inspect a DB container after a cornucopia error.

Perhaps we ought to add a cleanup_on_failure parameter to generate_managed that would default to true. What do you think?

[dbofmmbt]

I think it's a good idea to pass this option. Furthermore, I've been thinking about how to better expose this kind of configuration on cornucopia. Maybe a builder style would be more interesting, as it would be more flexible on adding new fields without breaking compatibility.

What do you think about it? This way, we could add new fields with default values without making users change their code.

ManagedGenerator::new(
    CodegenSettings::async_code(),
    vec!["schema.sql".to_string()],
)
.use_docker()
.destination("destination.out")
.generate()
.unwrap();

If you like the idea, I can finish the implementation and update this PR.

Changing subject... I noticed that there's some fields which are used as path to directories and files, but are typed as &str and String. If we're willing to break the API to make some cleanup, we could change them to Path or PathBuf to better represent their use.

[LouisGariepy]

Maybe a builder style would be more interesting, as it would be more flexible on adding new fields without breaking compatibility.

I really like this idea.

I noticed that there's some fields which are used as path to directories and files, but are typed as &str and String. If we're willing to break the API to make some cleanup, we could change them to Path or PathBuf to better represent their use.

I don't have a strong opinion on this, but I'm not opposed. As you mention, this could make the intent clearer.

Thanks for you contributions, much appreciated :smile:

[jacobsvante]

@LouisGariepy there's another reason one might want to use Path / PathBuf as well - it handles "OS strings" without you having to do any special handling. For this reason I think it's a no-brainer to use Path / PathBuf everywhere to represent paths.

If it's a user facing function one can also go the route of using P: AsRef<Path> / P: Into<PathBuf> to make it a bit more convenient.

[LouisGariepy]

Agreed!