Messages lost on apply

[CumpsD]

Currently you have the following:

    let inline apply wrappedFunction result = 
        match wrappedFunction, result with
        | Ok(f, msgs1), Ok(x, msgs2) -> Ok(f x, msgs1 @ msgs2)
        | Fail errs, Ok(_, msgs) -> Fail(errs)
        | Ok(_, msgs), Fail errs -> Fail(errs)
        | Fail errs1, Fail errs2 -> Fail(errs1)

When looking at Scott his ROP implementation he has:

let applyR f result =
    match f,result with
    | Success (f,msgs1), Success (x,msgs2) -> 
        (f x, msgs1@msgs2) |> Success 
    | Failure errs, Success (_,msgs) 
    | Success (_,msgs), Failure errs -> 
        errs @ msgs |> Failure
    | Failure errs1, Failure errs2 -> 
        errs1 @ errs2 |> Failure

Could we change it in Chessie to either one of these two:

Option 1

This is identical to Scott's version

    let inline apply wrappedFunction result = 
        match wrappedFunction, result with
        | Ok(f, msgs1), Ok(x, msgs2) -> Ok(f x, msgs1 @ msgs2)
        | Fail errs, Ok(_, msgs)
        | Ok(_, msgs), Fail errs -> Fail(errs @ msgs)
        | Fail errs1, Fail errs2 -> Fail(errs1 @ errs2)

Option 2

On fail, don't take the Ok messages along

    let inline apply wrappedFunction result = 
        match wrappedFunction, result with
        | Ok(f, msgs1), Ok(x, msgs2) -> Ok(f x, msgs1 @ msgs2)
        | Fail errs, Ok(_, msgs)
        | Ok(_, msgs), Fail errs -> Fail(errs)
        | Fail errs1, Fail errs2 -> Fail(errs1 @ errs2)

[CumpsD]

Option 2 feel more logical to me, since you can't tell which message was coming from Ok and which was from Fail, and most use case will display Fails on its own

[ReedCopsey]

Mixing Ok messages with fail messages seems like it'd make usability of the results much more complicated, as it wouldn't be obvious which messages were due to success and which due to fail, only that they switch part way through.

+1 @CumpsD - My thoughts exactly.

[theimowski]

IIRC from the sample app, the messages for Ok case were responsible to carry information about "events" that happened in the system - I'm not however sure what are the advantages of keeping them the same type as the Fail messages

[CumpsD]

Seems you share option 2 :) No advantages, but this does:

Fail errs1, Fail errs2 -> Fail(errs1 @ errs2)

So you don't lose errs2

[forki]

I'm for 2)

[pblasucci]

@CumpsD This is totally a bug. Good catch. The multiple failure messages should be getting merged together. As for merging the non-failing messages with the failing messages, I'm inclined to just drop the non-failing ones... but I think it's something that'll get sorted once we finally handle #17.

[CumpsD]

Shall I send a PR with the fix? On Apr 17, 2015 5:32 PM, "Paulmichael Blasucci" notifications@github.com wrote:

@CumpsD https://github.com/CumpsD This is totally a bug. Good catch. The multiple failure messages should be getting merged together. As for merging the non-failing messages with the failing messages, I'm inclined to just drop the non-failing ones... but I think it's something that'll get sorted once we finally handle #17 https://github.com/fsprojects/Chessie/issues/17.

— Reply to this email directly or view it on GitHub https://github.com/fsprojects/Chessie/issues/20#issuecomment-94022286.

[CumpsD]

There you go :)