Skip to content
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

Make open_browser take AsRef<OsStr>, add open_path function #38

Closed
wants to merge 3 commits into from

Conversation

philippeitis
Copy link

Since open_browser is the underlying implementation of open, I think making it provide an AsRef<OsStr> interface is acceptable (per the previous PR). However, open by default will only take AsRef<str>, while the new function, open_path, will only accept AsRef<Path>.

With regards to tests, is there anything in particular you would like to see? Would it be appropriate to add a simple test file and have all supported systems try to open this file?

@amodm
Copy link
Owner

amodm commented Feb 16, 2022

Closing this without merge, for the following reasons:

  1. I'd already mentioned earlier in Make open_browser take AsRef<OsStr> #35 that the APIs cannot be based on OsStr. They have to based on str, as we can guarantee conversion from str to OsStr, but not vice versa. This PR does not adhere to that request.
  2. While I was open to adding an open_path earlier, I realise upon thinking more deeply that the behaviour is going to be inconsistent. In some situations with the default browser, this will open up a file browsing application instead of the browser. As such, I'd like to wait for more feedback on it from the users before taking a call on it, as making future modifications to public facing APIs is a nightmare.
  3. My current thought on AsRef after a bunch of research, is to not include it, and keep it as &str. I've documented the reasons here

@amodm amodm closed this Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants