allow prefix to be any AsOsStr type, matching up with the contents of Path

[codyps]

No description provided.

[alexcrichton]

This was proposed some time back on the main repo, but I suggested against it as I didn't think that it was strongly motivated. Do you have a specific use case in mind where a strings do not suffice?

[codyps]

@alexcrichton it seems your suggestion against it in the PR was simply an issue with the API at the time (the switch for BytesContainer to OsStr was still in progress). Other than that, I can't find any statement of what would be problematic about this the PR you've linked.

As we now have AsOsStr, I'm not seeing how your previous concerns still apply.

[alexcrichton]

These were my specific concerns:

Ideally this bound would be AsPath or AsOsStr, but the OsStr one is a bit odd and it's not necessarily a Path but rather a suffix to a path. In light of this I think we'll want to stick to &str for now (the common language of paths as unicode), and we can generalize later if necessary.

[codyps]

Ideally this bound would be AsPath or AsOsStr, but the OsStr one is a bit odd and it's not necessarily a Path but rather a suffix to a path.

By "AsPath or AsOsStr", did you mean AsPath + AsOsStr? If so, I don't think that makes sense.
If not, it is AsOsStr in this PR.

but the OsStr one is a bit odd and it's not necessarily a Path but rather a suffix to a path.

So OsStr is not necessarily a Path? I suppose this might be true, but we construct paths from OsStr. For the purposes of this PR, the use of OsStr is simply to use something that can construct a path that has fewer restrictions than str

That said, it seems like the above sentence is saying "OsStr is a suffix to a path". That doesn't make sense, so I'll assume you meant "the argument is a suffix to a path". This is true, but it isn't a problem. OsStr can also be a path suffix, as demonstrated by the existence of the push method.

[alexcrichton]

By "AsPath or AsOsStr", did you mean AsPath + AsOsStr?

Ah sorry I meant either trait would be the bound, not both.

So OsStr is not necessarily a Path?

Strictly speaking all OsStr instances are also a valid Path, but I think it's gone pretty awry if you have something like:

TempDir::new(path, Path::new("suffix"))

This is true, but it isn't a problem. OsStr can also be a path suffix, as demonstrated by the existence of the push method.

Yes this PR is technically correct, but I believe it to be overengineered. The AsPath trait is relatively easy to understand and is used fairly commonly in the standard library whereas the AsOsStr is less common. In terms of a newcomer this makes the API a little more complex because you have to understand what OsStr is and how it interacts with Rust, both of which probably really aren't that necessary when you're just passing in a string literal.

[codyps]

I disagree, people who have managed to realize that AsPath exists (which they may as they are using the path API) are fairly likely to realize AsOsStr exists (as it is around for similar reasons).

That said: in all likely hood, those who don't know about the semantics there will still have no issue using an API that has AsOsStr as they will simply be passing a string literal and be none the wiser.

It is relatively simply not to over-restrict the type here. I don't see that as over engineering.

[alexcrichton]

I realize that the point about being over-engineered is subjective, but my original question still stands:

Do you have a specific use case in mind where a strings do not suffice?

[codyps]

I'm presently using a filename as the prefix for tempdir.

It's simply to communicate more information about the use of the particular tempdir to a user that stumbles across it.

It isn't possible (or rather, it would be rather silly to) round trip through utf-8 because then I'd potentially lose having directory prefix that in any way corresponds to it's usage.

[codyps]

It's been a month since any comment was made here, please do one of the following:

• provide new reasoning to convince me this is a bad idea
• accept my reasoning and merge the change
• disagree with my reasoning (or otherwise decide you don't want this in this particular tempdir crate) and decline the change

[alexcrichton]

Ah sorry for letting this fall through the cracks, I think this must have gotten lost in my email. This sort of change should always backwards compatible to do so I think I'd like to get some more experience without being so generic and see where it takes us.