Support workflow for persisting on panic

[vincentdephily]

Maintainer Edit: There are two problems that we became aware of in this issue

• Confusion over function names which was addressed by #56
• Wanting to support a workflow that auto-persists on panic which is still not resolved and this issue is focused on.

There are trade offs though. If someone is unaware, it could really eat up their disk space, for example.

Original title: TempDir::persist_if(false) should make sure that TempDir is not persisted.

Original body:

---

Currently, persist_if(false) returns immediately, assuming that the TempDir was not persistent to begin with and that nothing needs to be done. This doesn't work if persist_if(true) had been called earlier.

I'd expect an API that takes a bool to allow switching between two states, but this one can only switch from false to true.

Use-case: I'd like to automatically persist files for failed tests only, so that I can debug failures without bloating my disk. A simple way to do this would be to persist_if(true) at the start and persist_if(false) after asserts have passed:

#[test]
fn foo() {
    let td = TempDir::new().unwrap().persist_if(true);
    assert!(write_files_and_return_true(&td));
    td.persist_if(false);
}

[epage]

It was designed with the intention of the user being able to define an environment variable and use that to pass into persist_if which is why it accepts a boolean and why it has the _if rather than just being persist(yes: bool).

To un-persist, I guess we just have a bool to track that we want to manually delete it at the end?

Alternatively, what if we added a persist_on_panic? That'd save the hassle of doing arm/disarm?

[vincentdephily]

I thought that persist_if() would just convert between Inner::Temp and Inner::Persisted depending on the yes argument ? Or is there a technical reason we can't go back to Inner::Temp variant ?

A persist_on_panic would indeed simplify my usecase, but I wonder if it'd be less composable or miss out on some other usecase ?

You could try to cover all cases and future-proof the API a bit using something like

enum Persist{Yes,No,OnPanic}
impl From<bool> for Persist {...} 
impl TempDir {
    fn set_persist(self, impl Into<Persist>) -> Self {...}
    fn persist(&self) -> Persist {...}  //maybe useless
}

Regarding names, I don't really get the semantic difference between persist(bool) and persist_if(bool) for a setter. I'm happy with either, not going to bikeshed that one ;)

[epage]

I thought that persist_if() would just convert between Inner::Temp and Inner::Persisted depending on the yes argument ? Or is there a technical reason we can't go back to Inner::Temp variant ?

Yes, it is not reversible because we are telling the underlying TempDir to persist

        let path = match self.temp {
            Inner::Temp(temp) => temp.into_path(),
            Inner::Persisted(path) => path,
};

Regarding names, I don't really get the semantic difference between persist(bool) and persist_if(bool) for a setter. I'm happy with either, not going to bikeshed that one ;)

persist_if is not a setter which is why I didn't just name it persist(yes: bool). You aren't toggling a state but causing the persist to happen with the conditional built-in to streamline user code.

[volks73]

Might I suggest into_persist(bool) instead of persist_if since it is consuming the TempDir and there seems to be a Rust convention of into_ for method names that irreversibly consume the original?

Side note:

Alternatively, what if we added a persist_on_panic?

I love this idea! There have been a couple of times I wanted to persist temporary to know why a test failed. I wonder if this should be the default?

[epage]

Might I suggest into_persist(bool) instead of persist_if since it is consuming the TempDir and there seems to be a Rust convention of into_ for method names that irreversibly consume the original?

into seems like it'd help clarify the intent to avoid the confusion over whether disarm is possible. The main thing is if something should be in there to help convey the persisting is conditional (ie should we do into_persist or into_persist_if).

I love this idea! There have been a couple of times I wanted to persist temporary to know why a test failed. I wonder if this should be the default?

As for a default, I'm unsure. People unaware of this might start getting a lot of files sitting in their temp directory, especially if they have a test that is supposed to panic. However, it'd be a big help to people debugging.

[volks73]

into seems like it'd help clarify the intent to avoid the confusion over whether disarm is possible. The main thing is if something should be in there to help convey the persisting is conditional (ie should we do into_persist or into_persist_if).

I like into_persist_if since the method would take a boolean. I cannot find or think of an into_ method that has a parameter. They usually are just into_persist(), but I understand, and like, that the intention of the method was to associate it with some environment variable that can be used to toggle the behavior when running tests or based on an environment. Some words to this motivation for the method in the documentation may also alleviate confusion.

Would it be too much to have both?

[epage]

TO have both into_persist and into_persist_if? Nah, that sound fine.

[epage]

Only part of this has been addressed by #56, re-opening with the original issue updated with a summary of where we are at.