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

Use optional parameters instead of option in interfaces #130

Open
Shmew opened this issue Oct 25, 2020 · 8 comments
Open

Use optional parameters instead of option in interfaces #130

Shmew opened this issue Oct 25, 2020 · 8 comments

Comments

@Shmew
Copy link
Contributor

Shmew commented Oct 25, 2020

It's a little annoying to have to do add Some or None when calling instance methods for things like ICanTell<'Message>. These can be written using optional parameters which makes the API a bit easier to consume.

ICanTell<'Message> could be rewritten as:

[<Interface>]
type ICanTell<'Message> =
    abstract Tell : 'Message * IActorRef -> unit
    abstract Ask : msg:'Message * ?timeout:TimeSpan -> Async<'Response>
    abstract AskWith : f:(ICanTell<'Response> -> 'Message) * ?timeout:TimeSpan -> Async<'Response>

    abstract member Underlying : ICanTell

I don't mind submitting a PR, let me know what you think.

@Horusiath
Copy link
Owner

Do you use different timeout on every call? The common use case is to have a few timeout values that are specified as fields, and pass them around. In case when timeout is always the same, it can be set directly via HOCON akka.actor.ask-timeout = 5s entry (it's also used as default when no specific timeout value was provided).

@Shmew
Copy link
Contributor Author

Shmew commented Oct 28, 2020

Nah, it was mostly when I didn't care to specify a timeout, but I switched to using the operator more instead which passes None into the second parameter for me.

If this was implemented as optional parameters it would look a little better when piping the parameter to it.

myMsg |> tellable.Ask

vs

myMsg
|> fun msg -> tellable <? msg

I ended up just making a little module to do this for me:

[<RequireQualifiedAccess>]
module Actor =
    let tell (actor: IActorRef<'Message>) (msg: 'Message) = actor <! msg
    let ask (actor: IActorRef<'Message>) (msg: 'Message) = actor <? msg

@Horusiath
Copy link
Owner

If your goal if to pipe message from the left to actor on the right you could as well define: let inline (|?>) msg ref = ref <? msg.

@Shmew
Copy link
Contributor Author

Shmew commented Oct 28, 2020

That's a good idea! Funny enough right above that section I had a definition for (|!!>) for piping Tasks.

Any opposition to adding a |?> operator?

Although it's a bit "off" because |!> doesn't do what you might expect in that case since it's not a piped tell. It shouldn't be that difficult to operator overload to handle both cases, which could be done for all the operators. Is there ever really a circumstance where you'd send an actor Async<'T>? That wouldn't even work remotely, right?

@Horusiath
Copy link
Owner

Horusiath commented Nov 4, 2020

@Shmew I'm not really sure if incorporating this into library is a good idea. While anyone can add this operator in their own library and use in scope of their own project, IMO adding custom operators that are not obvious deepens confusion of developers. Even <! and <? are not immediately obvious if you have no knowledge of Scala version of Akka or Erlang.

Re. sending Async to an actor - tell/ask are specifically designed to convey message passing behavior. Async<> is not a message, it's a description of operation. Some languages (like Haskell) have dedicated operators (>>=) for sequential monadic bindings, but it's not the case here.

@Shmew
Copy link
Contributor Author

Shmew commented Nov 5, 2020

Yeah that makes sense.

So really it just comes back to making the timeouts optional rather than needing None passed in. Forgive me if I'm being dense, but is there a reason they would need to be explicit?

@Horusiath
Copy link
Owner

On the akka JVM providing the timeout parameter to ask method was mandatory - the reason is that you shouldn't have await for the operation that may potentially never finish (eg. because remote endpoint disconnected before reply). The rest is matter of API ergonomics - in Scala timeout param was defined as implicit, so it could be defined once and passed automatically by the compiler.

In C#/F# it's not that easy - the solution that would let us to have sane arg passing and still being able to use ? operator was to put timeout as optional parameter - in None case the timeout value would be fetched from HOCON configuration. There's no specific reason for timeout param to be explicit. Intention is that <? is the default way of calling Ask. Calling Ask as method (not operator) is opt-in variant, for situations where you need more control.

@Shmew
Copy link
Contributor Author

Shmew commented Nov 11, 2020

Yeah I get that, but is there a reason the second parameter needs to be explicitly None when using Ask? If they need to specify a timeout they can still do that, and in some cases makes it even easier to do so.

I'm fine with keeping the API the way it is if it's your preference or there's a reason for it. I'm just having a hard time understanding the point you're trying to make. This would not be a breaking change and make piping a message to an actor a lot more ergonomic in cases where you don't need to give an explicit timeout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants