-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Make std::io::TempDir::{new, new_in} accept a BytesContainer as a suffix #19447
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
Conversation
@@ -41,13 +41,24 @@ impl TempDir { | |||
|
|||
static CNT: atomic::AtomicUint = atomic::INIT_ATOMIC_UINT; | |||
|
|||
let suffix_str = match suffix.container_as_str() { |
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 the part that I'm unsure of. I like the idea of changing the parameter from &str to BytesContainer because that matches Path, but ultimately the suffix must still be a UTF-8 string, so the first solution that came to mind was to just return an error like this if that isn't the case, and proceed otherwise. But if we're just converting right to str anyway, does this commit make any sense? Thought I'd throw the PR out to start the discussion though.
a63e2c2
to
27ce252
Compare
@jmesmon Full disclosure, I'm still running make check on this revision. But this is already looking a lot cleaner to me, is this closer to what you were thinking? |
@aturon and I recently finished taking a close look at all of |
@alexcrichton Totally. I just saw the open issue and thought I'd take a stab at fixing it, but obviously please just do what you find to be best! |
27ce252
to
a556900
Compare
Thanks for being patient @csouth3! After thinking about with with respect to rust-lang/rfcs#517 I don't think that this is actually the direction that we want to go in. We've realized that paths are not actually piles of bytes but rather piles of bytes on unix and piles of u16 on windows. This means that eventually this is not the right bound for this function. Ideally this bound would be |
Ah, totally didn't realize the inconsistency between paths on unix/windows. Definitely makes sense to leave it then, thanks for the info! |
add more completion about "impl"
This pull request addresses #19437. It was mentioned that Paths can be constructed with any
BytesContainer
, whereasTempDir
's constructors specifically require an&str
. There's no reason thatTempDir
's constructors shouldn't be able to accept anyBytesContainer
however, so long as the suffix is a valid UTF-8 string.Closes #19437.