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 tasks from fsharpx #125

Closed
wants to merge 8 commits into from
Closed

use tasks from fsharpx #125

wants to merge 8 commits into from

Conversation

aprooks
Copy link
Member

@aprooks aprooks commented Jan 21, 2017

No description provided.

Signed-off-by: Alexander Prooks <aprooks@live.ru>
Signed-off-by: Alexander Prooks <aprooks@live.ru>
Signed-off-by: Alexander Prooks <aprooks@live.ru>
<* conflicts with same operator from Task library

Signed-off-by: Alexander Prooks <aprooks@live.ru>
@aprooks
Copy link
Member Author

aprooks commented Jan 24, 2017

I could make <* operator work as notify shortcut but only if it is implemented as global operator like:

[<AutoOpen>]
module ActorRef = 
   let inline (<*) (actorRef:^ref) (message:^msg) = 
      (^ref: (member Notify : ^msg -> unit) (actorRef, message))

also in client code open Orleankka.Fsharp should be strictly after open FSharpx.Task in order to this implementation work as expected. So I think it is better to rename operator

@yevhen
Copy link
Member

yevhen commented Feb 11, 2017

@aprooks what is the progress with this PR? Are you waiting on anything from me?

@aprooks
Copy link
Member Author

aprooks commented Feb 13, 2017

@yevhen yep I need your decision on whether we keep operators and if yes confirm notify operator as it conflicts with FSharpx

@yevhen
Copy link
Member

yevhen commented Feb 13, 2017

As I recall decision was to drop our since FSharp.Extras already provides these ops. Wrt to notify - we can just drop since it's extremely rate

@aprooks
Copy link
Member Author

aprooks commented Feb 13, 2017

ok I'll drop operators and fix conflicts in a day or two

Signed-off-by: Alexander Prooks <aprooks@live.ru>
Signed-off-by: Alexander Prooks <aprooks@live.ru>
Signed-off-by: Alexander Prooks <aprooks@live.ru>
@aprooks aprooks force-pushed the refactor/use-tasks-from-fsharpx branch from 3720631 to e1cb885 Compare March 3, 2017 13:07
@aprooks
Copy link
Member Author

aprooks commented Mar 3, 2017

@yevhen I think I like it even more. Verbose but reads easier.

@yevhen
Copy link
Member

yevhen commented Mar 3, 2017

it lost all coolness which F# guys love :)

@yevhen
Copy link
Member

yevhen commented Mar 4, 2017

In a order of reduced verbosity:

do! actor.Tell <| Hi
do! actor.Tell(Hi)
do! actor <! Hi
  1. Is most verbose but pipe provides better visual separation between receiver and message. Can quickly see message type when glancing over code
  2. Is shorter, but not visually separated
  3. Is the best. Both in terms of reduced verbosity and visual representation :(

What doesn't allow us to actually use 3) while still using FSharpEx.Tasks?
I think we can ask some F# gurus for help.

@AntyaDev
Copy link
Contributor

AntyaDev commented Mar 4, 2017

@yevhen

  1. One of the reasons to use our custom operators instead of FSharpx: ours do not populate global scope which basically helps us to handle the same operator (<*) for both: Task and ActorRef<'a>.
  2. I guess that F# users were able to use the Orleankka without operators from the beggining. Nothing stops them to write actor.Tell <| Hi
  3. I guess that the custom operators does make sense for Orleankka by emphasizing intent of the operation. Probably the same Send <| Msg will be used to work with Kafka, Rabbit and so on.
  4. I guess that custom Task module gives you ability to express some interesting concepts: streams, flows and so on. My intuition is saying me that better to keep Task module as Orleankka part (at least for now).
    Take a look on: Elixir Flow and GenStage.

@aprooks
Copy link
Member Author

aprooks commented Mar 4, 2017

@yevhen What doesn't allow us to actually use 3) while still using FSharpEx.Tasks?

the only conflict issue was with Notify operator. It has different return type and thus gets overriden by FSharpX if library is opened after Orleankka. So if we remove only this one or find another one we should be good to go.

I guess that custom Task module gives you availability to express some interesting concepts: streams, flows and so on. My intuition is saying me that better to keep Task module as Orleankka part (at least for now).

There are no any new concepts implemented atm, aren't there?

My main issue with duplicated code is following. If someone uses library depending on original FSHarpEx he'll probably have some fun time resolving issues arising after that.

@AntyaDev
Copy link
Contributor

AntyaDev commented Mar 4, 2017

@aprooks @yevhen

There are no any new concepts implemented atm, aren't there?

I mean that you can add them if you will. Some kind of monad transformers to use Result (Railway) and compose data processing flows.

My main issue with duplicated code is following. If someone uses library depending on original FSHarpEx he'll probably have some fun time resolving issues arising after that.

Dude, do not overestimate DRY principle. I have seen this a lot especially in OOP, when in order to save 1 method someone just adds an additional layer of inheritance via BaseClass. It's just adding complexity.
Also take into account that you have a type alias in F#, otherwise you end up with one extra function like convertToTask which will be used in a few places where you will mix some FSharpX task with Orleankka.
On top of my head, I can give you several solutions how to prevent mixing FSharp task with your own. Don't forget about the context of using actors.

And as I mention already - My intuition is saying me that better to keep Task module as Orleankka part (at least for now).

@aprooks
Copy link
Member Author

aprooks commented Mar 4, 2017

when in order to save 1 method someone just adds an additional layer of inheritance via BaseClass I've seen 11 levels inheritance once created to reuse few fields :)

I don't have much experience with F# so I am happy to follow your suggestions.

if @Yevgen agrees with you this PR should be closed.

@yevhen
Copy link
Member

yevhen commented Mar 12, 2017

@aprooks agree. Thanks, @AntyaDev for sharing your thoughts.
Closed!

@yevhen yevhen closed this Mar 12, 2017
@yevhen yevhen deleted the refactor/use-tasks-from-fsharpx branch April 6, 2017 19:27
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

Successfully merging this pull request may close these issues.

None yet

3 participants