Skip to content
This repository was archived by the owner on Aug 25, 2022. It is now read-only.

connect returns no error#71

Merged
yomimono merged 1 commit intomirage:masterfrom
hannesm:error
Oct 2, 2016
Merged

connect returns no error#71
yomimono merged 1 commit intomirage:masterfrom
hannesm:error

Conversation

@hannesm
Copy link
Copy Markdown
Member

@hannesm hannesm commented Oct 1, 2016

No description provided.

@Drup
Copy link
Copy Markdown
Member

Drup commented Oct 1, 2016

The reason it's return and not Lwt.return everywhere is that functoria doesn't depend on Lwt, and we expect a prelude provided by the application. We could revisit that, but it doesn't seem necessary here.

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Oct 1, 2016

@Drup maybe functoria does not explicitly depend on Lwt, but certainly the current master (a3c5d3b) already emits Lwt.return sometimes (see e.g.

"@[%s.start@ %a@ >>= fun t -> Lwt.return (`Ok t)@]"
)

I think we have to be honest that functoria does actually implicitly plays well with Lwt only atm (since nobody tried anything else) -- and I find it much less confusing to emit what we mean. Additionally, in case you want to use functoria without Lwt, you only have to provide a mockup module Lwt which contains some return.

@Drup
Copy link
Copy Markdown
Member

Drup commented Oct 1, 2016

Hum, I missed that Lwt.return, it shouldn't be there.

Fair enough, but in this case, add the dependency, fix the documentation and use Lwt.fail instead of failwith in the runtime. ;)

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Oct 1, 2016

OTOH, I think you're right, and the documentation is well written on that part :) I force-pushed to get rid of Lwt.

@yomimono yomimono merged commit d6a3bd3 into mirage:master Oct 2, 2016
@hannesm hannesm deleted the error branch October 2, 2016 19:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants