Closed hannesm closed 4 years ago
It was changed because then it would fit better with Lwt.async
indeed. I can't remember exactly where this was an issue, I believe it was somewhere in a hacky build script that injected strings into unikernel.ml
through some fields that mirage
failed to escape correctly.
I don't see the need to break it again, and if remove the unit
argument, now all the consumers will invariably have to write Lwt.async (fun () -> GUI.listen t)
instead of Lwt.async (GUI.listen t)
.
It would make sense to make the 'a
a unit
as you suggest:
val listen : t -> unit -> unit Lwt.t
Although I slightly prefer the original API, I don't think we should change it again. Breaking APIs just over style disagreements seems silly, and both APIs are fine.
However, GUI.listen
should still return 'a Lwt.t
to show that it doesn't return. It's just a coincidence that Lwt.async
happened to have a similar-looking 'a
in its type. They're not the same, because of the implicit forall. i.e. we really had:
val async : 'a. (unit -> 'a Lwt.t) -> unit
val listen : ('a. unit -> 'a Lwt.t)
async
's (old) type said it doesn't matter what the function returns. listen
's type says it doesn't return.
I'm happy with 'a
and @talex5's reasoning.
since @talex5 prefers to not change the API, let's close this.
it is not clear why this was changed in https://github.com/mirage/mirage-qubes/commit/2393d8d5b937139fdf705d1defca067c9eb7eadc //cc @cfcs -- was there a good reason to introduce the
unit
argument?I know this has been released since then (i.e. this PR breaks the API again), but that's fine. (see as well @reynir comment at https://github.com/mirage/mirage-qubes/commit/2393d8d5b937139fdf705d1defca067c9eb7eadc#r34791187) -- and with lwt 5.0.0 being released now (where
Lwt.async
requiresunit -> unit Lwt.t
), should the return type ofval listen
("it never returns") beunit Lwt.t
instead?