Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

connect returns no error #71

Merged
merged 1 commit into from Oct 2, 2016
Merged

connect returns no error #71

merged 1 commit into from Oct 2, 2016

Conversation

@hannesm
Copy link
Member

@hannesm hannesm commented Oct 1, 2016

No description provided.

@Drup
Copy link
Member

@Drup 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
Member Author

@hannesm 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
Member

@Drup 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 hannesm force-pushed the hannesm:error branch from 6b40457 to 6693093 Oct 1, 2016
@hannesm
Copy link
Member Author

@hannesm 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
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@hannesm hannesm deleted the hannesm:error branch Oct 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants