feat(riot): add run_with_status function where main can return a status code/error

[Dev380]

This attempts to resolve #50

Sorry if anything here is wrong/misunderstanding the issue, I'm still a bit new to OCaml.
I created another function, run_with_status, which implements the expected behaviour to avoid making a breaking change. I didn't know what the error type should be, and the issue said to make it a polymorphic variant, so I added a placeholder `Unknown variant for now. I'm assuming it should accept an io_error, but that requires moving the run functions down ~200 lines so I want to make sure that's okay before doing that. Running dune test in the root directory seems to suggest nothing has broken.

I implemented the regular run function in terms of run_with_status. I don't think that's optimal for performance if run is going to be used in production, should I change that so run_with_status is a wrapper around run? Currently, the two run functions are swapped around in the mli, and I just noticed that everything else is in the same order in the interface and implementation files. Is this a convention that I should follow or just coincidence?

Also, should I write a test for this function?

[Dev380]

I changed it around for now so the run function should be untouched and run_with_status just wraps it.

[Dev380]

I forgot that #50 asked for it, run_with_status should use `Msg of string instead of `Unknown now.

[leostera]

suggestion. This wrapping looks good but we can improve it a little bit since if main () returns an error we will have a runtime exception and the application will crash without shutting down the engine correctly.

The right way of handling this would be to pattern-match on main () and handle both the Ok status case and the Error reason case.

For the Ok case, your current code looks good.

For the error case, we will need to ask the user how to handle it. We can this by also accepting a new parameter like on_error which by default is defined as a function that handles `Msg string errors:

let default_on_error (`Msg reason) = 
  (* ... print error msg ... *)

This would allow a user to also pass in their own function if they want to have other kinds of errors handled.

What's important is that after calling their function, we always call shutdown ().

[Dev380]

I implemented this but I don't know what to do about cases where the error is not `Msg of string (currently it just says the error is unsupported for printing).

[leostera]

Left you a few comments that should help with this :)

[leostera]

Hi @Dev380, thanks for opening the PR! ✨

@Dev380: Currently, the two run functions are swapped around in the mli, and I just noticed that everything else is in the same order in the interface and implementation files. Is this a convention that I should follow or just coincidence?

I normally generate the first draft of the .mli using the ocaml-lsp-server, which preserved the order of the .ml file. But this is not needed, and we can move those functions around to make the .mli and the docs of a module easier to read / digest.

Left you a comment, after we address this we should be good to merge! :)

[Dev380]

98b5592 should resolve the crashing without shutting down, should run_with_status also catch exceptions and/or use io_result instead of [> `Msg of string] or is it fine as-is?

[leostera]

suggestino. If we are more general about the 'error that we return and how to handle it, we can have a smaller error type for our default_on_error function.

Suggested change

	val run_with_status : ?rnd:Random.State.t -> ?workers:int -> ?on_error:(([> `Msg of string ] as 'a) -> int) -> (unit -> (int, 'a) result) -> unit
	val run_with_status : ?rnd:Random.State.t -> ?workers:int -> ?on_error:('error -> int) -> (unit -> (int, 'error) result) -> unit

This change should mean that you can return whatever error type you want, as long as you use an on_error function that supports it. If you use the default, then the only error type you can return is [|Msg of string ] `.

[Dev380]

Oh I see, thanks :D

[Dev380]

I'm still getting [ `Msg of string ] not captible with 'error in the mli file unfortunately