Make open_browser take AsRef<OsStr>

[philippeitis]

I wanted to use this to open files in the web browser, but as it turns out, it only accepts strings. I was quite surprised to see this, so I added code to allow using anything implementing AsRef instead, which should support both url and filepath usecases.

I have tested this on Unix and it works perfectly. I'd also like to add support for the major browsers on Windows/Unix, as I believe it should be as simple as doing "firefox path" or "chrome path"

[amodm]

Welcome, your first PR here :) Moving this to AsRef makes sense to me, but ideally I'd like to not expose out a std::ffi based signature. Any suggestions on better alternatives?

[amodm]

Welcome, your first PR here :) Moving this to AsRef makes sense to me, but ideally I'd like to not expose out a std::ffi based signature. Any suggestions on better alternatives?

@philippeitis gentle ping.

[philippeitis]

Hi - I don't really think there is a better alternative, since handling file paths will always require a type from std::ffi. Most other libraries which provide this functionality also expose std::ffi. Additionally, every single OS where this library would be useful provides std::ffi and the str -> OsStr conversions, so end-users should not be affected by this change. Likewise, Command also takes AsRef<OsStr> args, so implementation-wise, very little changes.

[amodm]

@philippeitis, I thought about it. Semantically, I don't think it's correct to expose OsStr. I understand that there'd be a bunch of different crates comfortable exposing a OsStr, but there's a reason why both OsStr and str exist.

Path exposes OsStr because semantically it is representing a filesystem path, which is an OS construct anyway. OTOH, a URL isn't exactly an OS construct. A good way to think about it is that str to OsStr conversion is guaranteed, but not vice versa. That makes OsStr a poor choice for external API of this library, even if internally it might still be using OsStr.

Given that it isn't too hard to convert a Path to a str, which solves for the required use case, I'm deciding against merging this PR.

I do like exposing an AsRef<str> though, instead of the current <&str>, for some of the reasons you mentioned. If you'd like to contribute to that change, I'll be happy to take a PR for it.

[philippeitis]

That's disappointing to hear, and I hope you'll reconsider on some points.

The reason I sent this PR is because in general, it is not actually possible to convert a Path to a str losslessly. There are only two methods which convert PathBuf to str - to_str, which returns Option<&str> (and so you'd not be able to open a valid file), and to_string_lossy, as you might imagine, is lossy and also does not accomplish my goal. Accordingly, I'd need to have to add some form of error handling for a case which can be handled without any errors in the underlying implementation, which is less than ideal.

Links:
https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.to_str
https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.to_string_lossy

It is nice to be able to open HTML or SVG files in a browser by default, and it would also be nice to be able to take advantage of browser detection to select an available browser. This is my intended use case, and not having access to an OsStr interface makes this library considerably less useful for me. I would greatly appreciate having the option to open files (eg. via a new open_path which maybe accepts AsRef<Path>), even if you decide that by default, open should not do this.

In particular, changing the signature of open_browser_internal to accept OsStr seems like a reasonable choice, since the name suggests that it is

1. Internal to the crate and should maybe present an API which exposes more of the implementation details, as opposed to the open function, which may obscure these for clarity and ease of use
2. intended for users who need more control (eg. me) over how they are using the underlying functionality (eg. using OsStr instead of str)

[amodm]

Having an open_path as an additional function is perfectly fine by me (with corresponding tests).

OsStr not being a guaranteed conversion to str is actually the reason why I'm saying it would be a bad design to expose in the API, because technically, a string that isn't a valid url can be passed to this library. This discussion made me realise that even a str isn't a good choice, should've been a url::Url instead, but that change will be invasive in the current signature, so not doing that right now. But I certainly don't want to regress from here.