feat: add commands function #64
Conversation
matoous
force-pushed
the
md/command
branch
7 times, most recently
from
b20eff3
to
fc6c21b
Compare
Adds new `command` function that returns the underlying `std::Command` that would be called to open given path. Related: Byron#63 Related: helix-editor/helix#5820
There was a problem hiding this comment.
Thanks for giving it a shot!
Instead of trying --version in various programs, I think it's better to try to execute them. That way, Command tells you if it could find an executable by that name by erroring when trying to launch it, or not. This assumes these programs don't have a side-effect when launched like that which I believe should be the case. That, however, definitely needs validation.
Once a program is found, one could cache this information statically, but then again, we didn't do that previously and can probably just keep everything pure and simple. In the worst case, we would probably waste 40ms to find a program that works, which seems quite alright.
|
@Byron done. Any recommendations for how to easily fix this for someone who runs solely on macos devices? I could spin up a few VMs but maybe there's easier way? |
|
I think we actually should neither call version or execute the command in anyway. Instead we should just return a vec of commands that can be executed by the calle until one of then works. Calling any command in anyway is a blocking operation and should not be done unless explicitly requested. |
I am using Parallels for that, and am not aware of an easier way except for using CI, but that has its own drawbacks. Once the API is finalized I am happy to help test it on some linux VMs I have available, and windows as well. |
Fair enough! Let's not return a |
src/haiku.rs
Outdated
| .arg(path.as_ref()) | ||
| .status_without_output() | ||
| .into_result() | ||
| command(path).status_without_output().into_result() |
There was a problem hiding this comment.
I think this CommandExt method should be split into two, so the configuration of IO channels is separate from executing it.
Thus, future invocations would look more like this:
Command::new(…).without_io().status().into_result()
src/ios.rs
Outdated
| @@ -2,12 +2,14 @@ use std::{ffi::OsStr, io, process::Command}; | |||
|
|
|||
| use crate::{CommandExt, IntoResult}; | |||
|
|
|||
| pub fn command<T: AsRef<OsStr>>(path: T) -> Command { | |||
There was a problem hiding this comment.
The signature here could be
| pub fn command<T: AsRef<OsStr>>(path: T) -> Command { | |
| pub fn commands<T: AsRef<OsStr>>(path: T) -> impl Iterator<Item = Command> { |
Note that Option implements Iterator, so single-commands can be wrapped in an option when returning them.
src/lib.rs
Outdated
| @@ -131,6 +137,18 @@ pub fn with<T: AsRef<OsStr>>(path: T, app: impl Into<String>) -> io::Result<()> | |||
| os::with(path, app) | |||
| } | |||
|
|
|||
| /// Get command that opens path with the default application. | |||
There was a problem hiding this comment.
I think it should be mentioned that this is a 'no-run' sibling of open::that(…), as opposed to, say, open::with(…).
matoous
changed the title
matoous
force-pushed
the
md/command
branch
4 times, most recently
from
02fce60
to
7f5be89
Compare
|
I am closing this (somewhat stalled) PR in favor of #66 which incorporates some changes from this PR while rounding out the new The reason for my sudden activity is #65 which made me think that this PR actually contains a solution already, so I'd like to see it come to completion now once and and for all. Thanks for everybody involved - and thanks to |
Adds new
commandsfunction that returns iterator overstd::Command. This exposes the commands used bythatto the callee so that they are able to run the command on their side.Related: #63
Related: helix-editor/helix#5820