quote open command with Filename.quote_command and return Lwt process

[nguermond]

Addresses issue #1.
Changes:

• return an Lwt process
• use Filename.quote_command instead of sprintf to quote
• use Lwt_process.exec builtin functionality to redirect output to /dev/null

I similarly ran into the issue that strings were incorrectly quoted. For example, the original code produced

Format.sprintf "%S" "hél'lo world";;
- : string = "\"h\\195\\169l'lo world\""

but the system does not recognize this filename. We should instead have

Filename.quote "hél'lo world";;
- : string = "'hél'\\''lo world'"

[nguermond]

Alternatively, you could add a wrapper in_default_app : string -> bool for backwards compatibility?

[smolkaj]

Thanks for the PR, Nathan. Very nice to get the quoting issue fixed and to have Lwt support.

Two requests:

1. I would like to keep the non-Lwt version around as well, for those of us who do not use it
2. Can we make the Lwt dependencies opt-in, so it's only required for people who wish the Lwt version of the function?

Regarding (2), unfortunately I am not an expert on how to best achieve this since I don't use Lwt/Async myself, but I know that many libraries provide multiple packages to achieve this somehow...say open and open.lwt/open_lwt. Perhaps you would know?

Regarding (1), perhaps Open.in_default_app and Open.in_default_app_with_lwt / Open.WithLwt.in_default_app or something like that? I don't feel strongly, just footing ideas.

[nguermond]

I factored this in two packages, open and open_lwt. The interface is now

val in_default_app : string -> bool (* or bool Lwt.t *)
val in_default_app_status : string -> Unix.process_status (* or Unix.process_status Lwt.t *)

I also had to update the tests, which now build conditionally on dependencies. (I couldn't figure out the Core dependency so I just replaced some of the code with Stdlib builtins).

[smolkaj]

Thanks, this looks great! Just a few nits / comments.

[smolkaj]

Nit: remove empty line.

[smolkaj]

Same for the other new files

[smolkaj]

Out of curiosity, why do we neee OCaml 4.14 here?

[smolkaj]

Feel free to add yourself as an author.

[smolkaj]

Nit: let ()

[smolkaj]

Same for the let version

[smolkaj]

Unfortunately the continuous integration seems to no longer be working, I opened #3 to track that. I can give this PR a try manually so we don't have to block on that issue.

[smolkaj]

Unfortunately the continuous integration seems to no longer be working, I opened #3 to track that. I can give this PR a try manually so we don't have to block on that issue.

It works fine on my machine, macOS, with Ocaml 5.0

[nguermond]

Actually I've been having issues getting dune to skip the examples when lwt is not installed, so I will have to find a way around that.

[smolkaj]

Unfortunately the continuous integration seems to no longer be working, I opened #3 to track that. I can give this PR a try manually so we don't have to block on that issue.

Fixed

[smolkaj]

Actually I've been having issues getting dune to skip the examples when lwt is not installed, so I will have to find a way around that.

I actually think that might be fine? I just don't want the non-Lwt opam package to depend on lwt, and you have achieved that AFAIK.

[nguermond]

I'm afraid opam CI will complain (but not sure).
Funnily enough this pull request for dune from a few days ago would fix this issue if you're willing to wait for it to be merged and depend on the latest version of dune.
Otherwise I can just remove the lwt tests for now.

[smolkaj]

I'd be happy either way. No rush on my side.

[smolkaj]

Looks like dune 3.9 with the patch you mentioned just got release :)