-
Notifications
You must be signed in to change notification settings - Fork 122
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
Add stored parent_dir builder parameter #308
base: master
Are you sure you want to change the base?
Conversation
I have a usecase where it's useful to be able to pass `Builder`s around and have them remember the parent directory to create files in, just like they remember the filename prefix and suffix. So, add a new `parent_dir` API to do just that. Note that we still call env::temp_dir even when the result won't be used. Using `self.dir.unwrap_or_else(|| temp_dir())` doesn't work because `tempdir_in` doesn't accept an owned `PathBuf`. If anyone prefers, we can easily get rid of this pointless call by just having an explicit `match` instead of `unwrap_or`, but it's a bit more verbose. No strong feelings either way from my side.
After writing this and then trying it out (see example usage here) I realised that a) Unless I'm mistaken this is an incompatible change because it added a new lifetime parameter to b) Because the But, I'm pretty new to Rust, so I thought I'd send this PR despite these problems, in case it turns out to be useful anyway e.g. because I'm misunderstanding these issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me.
@@ -461,7 +485,7 @@ impl<'a, 'b> Builder<'a, 'b> { | |||
/// [security]: struct.NamedTempFile.html#security | |||
/// [resource-leaking]: struct.NamedTempFile.html#resource-leaking | |||
pub fn tempfile(&self) -> io::Result<NamedTempFile> { | |||
self.tempfile_in(env::temp_dir()) | |||
self.tempfile_in(self.dir.unwrap_or(&env::temp_dir())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a documentation update here and below.
@@ -304,6 +306,28 @@ impl<'a, 'b> Builder<'a, 'b> { | |||
self | |||
} | |||
|
|||
/// Set a directory to create files in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Set a directory to create files in. | |
/// Set the default directory in which temporary files/directories will be created. |
@@ -171,29 +171,31 @@ pub use crate::spooled::{spooled_tempfile, SpooledData, SpooledTempFile}; | |||
|
|||
/// Create a new temporary file or directory with custom parameters. | |||
#[derive(Debug, Clone, Eq, PartialEq)] | |||
pub struct Builder<'a, 'b> { | |||
pub struct Builder<'a, 'b, 'c> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change. IMO, I messed up and should have had just one lifetime here (relying on sub-typing) but fixing that would also be a breaking change.
We can do one of:
- Re-use one of the other lifetimes.
- Store the directory by-value (although I'd still take the parameter as
AsRef
).
I'd go with the former approach (probably using the prefix's lifetime, 'a
. It's a bit awkward but it provides the best upgrade path for an eventual v4.
Ah, I missed your comment.
Yes.
Also yes. It's possible to pass it around, but a bit awkward. It would be possible to erase the lifetime entirely by making the builder own the prefix etc.... but I'm trying to reduce allocations a bit. The builder is designed to be an ephemeral way to construct temporary files/directories. |
Thanks for the input :) So I think this feature is probably not worth it as-is, I think anyone who would benefit from it is pretty likely gonna want to end up creating their own wrapper type anyway which is what I ended up going for in my case. Maybe if it was a part of a broader evolution it might make sense though. It's unclear to me how feasible it is to create something like the builder but which is generic wrt whether it owns its params or just has a reference. My understanding is that Do you think I'm on the right lines there? In that case I think I should abandon this effort. (BTW also I'm mostly just shooting the breeze at this point, if you're too busy to help random strangers understand Rust API design principles please feel free to ignore me!) |
Learned something interesting, I ran out of time to fully explore it so I'll just drop some notes...
I think pub struct Builder<'a, 'b> {
// ...
prefix: Cow<'a, OsStr>,
suffix: Cow<'b OsStr>,
// ...
}
impl<'a, 'b> Builder<'a, 'b> {
pub fn new() -> Self {
Self::default()
}
pub fn owned_prefix(&mut self, prefix: OsString) -> &mut Builder<'static, 'b> {
self.prefix = Cow::Owned(prefix);
self
}
} But this turns out to fall afoul of some pretty hardcore type system business:
But I think the hardcore stuff about variance is probably a red herring and this is just me expecting the borrow checker to do impossible magic, if you have
I think I need to meditate at the top of a mountain and understand that error then I'll be able to understand if there's actually a solution here or not. |
I played with that and got it working on my local "eventual v4 branch" but... it's not worth it, IMO. What I did was:
Unfortunately, that static builder has all kind of thorns because, e.g., calling IMO, the right approach is to pass around a closure or a custom temporary file creator. |
(although I may consider in an eventual v4) |
I have a usecase where it's useful to be able to pass
Builder
s around and have them remember the parent directory to create files in, just like they remember the filename prefix and suffix.So, add a new
parent_dir
API to do just that.Note that we still call env::temp_dir even when the result won't be used. Using
self.dir.unwrap_or_else(|| temp_dir())
doesn't work becausetempdir_in
doesn't accept an ownedPathBuf
. If anyone prefers, we can easily get rid of this pointless call by just having an explicitmatch
instead ofunwrap_or
, but it's a bit more verbose. No strong feelings either way from my side.