use persist() instead of into_path() for temporary directories

[vitiral]

I opened this issue at rust-lang-deprecated/tempdir#44 and was requested to open it here instead.

I have copy/pasted the original issue below:

---

Feature Request

This is a feature/API change request to rename into_path() to persist(), while deprecating into_path().

Reasoning

While I understand the logic behind calling it into_path() (since it "converts" the temporary directory into an "ordinary path"), into_path() actually performs an action from the programmers point of view, namely it removes the invariant that the temporary directory will be deleted on drop.

This is made even more confusing by the documentation, which says: "This destroys the TempDir without deleting the directory represented by the returned Path." -- from a lingual point of view it sounds like we are destroying the directory

Solution

/// Consumes the `TempDir` object without deleting the directory, 
/// returning the `PathBuf` where it is located.
///
/// The directory will no longer be automatically deleted.
fn persist(self) -> PathBuf { self.into_path() }

[Stebalien]

My one concern is that we already have a persist(location) method on NamedTempFile that takes the location at which to persist the temporary file and I'd like the version on TempDir to do the same. How would you feel about persist_in_place(), or something like that?

Out of curiosity, what is your use-case for this? I've never added such a function to NamedTempFile because I can't really think of a case where I'd want to persist a temporary file/directory in-place (with the generated random name).

[vitiral]

The main usecase it to remove a confusing API (into_path invalidating the invariant that you delete the tempdir).

Usecase: I have to have to store logs/artifacts somewhere but don't care where.

persist(path) is interesting, I didn't think about passing a path to the method -- I really like that! I'm not a huge fan of the name persist_in_place though.

Another possible solution: it is possible that into_path could just have improved documentation.

The current docs are:

Unwraps the Path contained in the TempDir and returns it. This destroys the TempDir without deleting the directory represented by the returned Path.

Maybe this could be changed to:

Consumes the TempDir without deleting the filesystem directory and returns the PathBuf where the directory is located.

The directory will no longer be deleted automatically.

[Stebalien]

I agree, regardless of what we do the docs can be improved. Mind submitting a PR?

[vitiral]

One possible suggestion, it seems like persist(path) is actually the wrong name to me. Maybe consider move(path) and the docs clearly state that it won't handle deleting it anymore?

The only issue is that it is kind of weird to "move" a TempFile since the file doesn't technically exist yet (kind of...), but I think that would make more sense.

This would allow persist() to exist for what I describe.

[Stebalien]

Persist does more than move tempfiles. It makes them no longer temporary. On windows, this involves changing the file attributes. Also, move doesn't indicate that we're making the file persistent in any way.

Now, I could change persist to persist_at and rename into_path into persist (and mirror that method on temporary files) but that'll be an even bigger breaking change (yes, I'm bumping the version but I'd like upgrading to be relatively painless). I'll have to think about this a bit.

[vitiral]

That's a good point.

What if TempDir::persist was persisit(Option<AsRef<Path>>)? It's not the end of the world for them to have slightly different APIs.

[Stebalien]

I'm really not a fan of using options for optional parameters like this.

It's not the end of the world for them to have slightly different APIs.

Except that I'll see it every time I look at the API and kick myself. I hate inconsistent APIs. I'd rather rename persist to persist_at than have that.

[vitiral]

@Stebalien neither am I ☹️

maybe we can come up with a better name. Let's sleep on it.

[Stebalien]

So, every way I can think of fixing this causes more harm than it helps (breaks a lot of existing code while leaving the API inconsistent). Unless anyone has any bright ideas before the end of the week, I'm planning on cutting a release.

[vitiral]

I think rename it to persist_at would be pretty good personally.

[Stebalien]

As a solution, yes. I'm just worried that the amount of breakage will dwarf the benefit.

[vitiral]

another solution, TempFile could have persist(path) but the others could have just persist(). The others already exist so they don't need a path but TempFile does because it doesn't exist yet. This would allow for no breakage but wouldn't be too confusing I would think.

[Stebalien]

So, there's the temporary file returned by tempfile(). This one doesn't exist in the filesystem but, unfortunately, can't exist on some systems (i.e., we can't link the file into the filesystem). Then there's NamedTempFile. That one does exist, just like TempDir.

[vitiral]

oh I see. And making NamedTempFile.persist not take an argument would be a breaking change...

I'm beginning to like a better documented into_path() more and more, 😆

[Stebalien]

For now, I've just cut a release. I agree that the naming choice here was a bit unfortunate but, it still sort of makes sense and leaving it the way it is minimizes breaking changes.