Fail active recognizer shadows Fail union case

[mexx]

The Fail active recognizer introduced in #15 by @pblasucci shadows the Fail union case. It actually introduces "incomplete pattern matches on this expression" compiler warnings for code like

match result with
| Ok (x, msgs) -> ...
| Fail (msgs) -> ...

Prior to this change the match was complete. Fortunately the change only confuses the compiler and do not change the behavior of the pattern match.

We should require qualified access on one of the two constructs or rename one of them to avoid the name clash.

[pblasucci]

So we've got a few options:

1. Add RequireQualifiedAccess to the Trial module (because the attribute is invalid on Active Patterns)... this will impact all of the other combinators
2. Add RequireQualifiedAccess to the Result union
3. Rename Ok | Fail to Ok | Bad (or something else?)
4. Rename Pass | Warn | Fail to something (I'm at a loss because "pass" and "fail" are natural antonyms)
5. Something else of which I haven't thought

I'm honestly a fan of the option 3. But, since changing Result is something people have had feelings about in the past, I will put to anyone interested as an informal poll.

Opinions?

[forki]

@vasily-kirichenko what do you think about 3? I think this is a good solution.

[forki]

see https://github.com/fsprojects/Chessie/pull/19

[vasily-kirichenko]

@forki I'm not sure I understand the problem. Our team use the following with no problem:

let inline Ok a: Choice<_, _> = Choice1Of2 a
let inline Fail a: Choice<_, _> = Choice2Of2 a

let (|Ok|Fail|) = function 
    | Choice1Of2 a -> Ok a
    | Choice2Of2 a -> Fail a

[pblasucci]

@forki @vasily-kirichenko the ok and fail functions aren't at issue. We have an Active Pattern shadowing one case of the underlying DU (because we went with a specific DU rather than aliasing Choice<'a,'b>).

[vasily-kirichenko]

Maybe aliasing is a good idea? It'd enable many existing functions and computation expression builders from various libraries.

[forki]

this would mean that we don't have messages in the success case. I actually don't think we need that. /cc @mexx @theimowski

[theimowski]

we could leave messages in success case and do sth like

let inline Ok a: Choice<_, _> = Choice1Of2 (a, [])

are the messages in success necessary? I copied them over, here's a sample use case for those: https://github.com/swlaschin/Railway-Oriented-Programming-Example/blob/master/src/FsRopExample/DataAccessLayer.fs#L204-L205

[forki]

in that cases we would just change the Success result type to a tuple, right?

[theimowski]

I think it's a tuple already: https://github.com/fsprojects/Chessie/blob/master/src/Chessie/ErrorHandling.fs#L9

[forki]

no I mean we would make 'TSuccess a tuple (which then contains the message)

[theimowski]

Ahh yes

[forki]

so I propose we change that to what @vasily-kirichenko is using.

[pblasucci]

If we drop the extra messages from Ok, what does that mean for Pass|Warn|Fail?

[mexx]

To drop or not to drop, that's a question.

For me messages of success track can be seen as a process log. Maybe we need simply a data type for this concept?

type WithLog<'Value, 'Message> = 'Value * 'Messages list

Currently the error messages and process log messages must be of same type, but is it always the case? Furthermore most of the time I don't need the process log at all, but it's there and interferes every time I only want the result, besides of using returnOrFail.

With this concept we could go the aliasing way and let the user decide whether and when to add process log to the code.

If we drop the extra messages from Ok, what does that mean for Pass|Warn|Fail?

Well with the extra concept for process log those recognizers would work with Result<WithLog<'Success, 'Message>, 'Message> type or an alias ResultWithLog<'Success, 'Message> = Result<WithLog<'Success, 'Message>, 'Message>.

[pblasucci]

So, I played with a bit with @mexx's idea, and incorporated @vasily-kirichenko's suggestion of just aliasing Choice<'TSuccess,'TFailure>. I have to say, I think I've missed something.

Removing the "process log" and the list of failure messages basically leaves us with a nearly straight implementation of what already exists in ExtCore (and probably FSharpX, though I haven't checked). If that's what folks want (or do they really just want Haskell?), then I'm not sure there's much merit in Chessie.

[forki]

I think we're still in the early phase. What about another experiment: what if we created a third union case for the warn stuff?

[vasily-kirichenko]

What about me, I'm not going to use Chessie since I'm totally happy with the Either monad. So, don't get my opinion seriously.

[mexx]

With third union case all users would be forced to care about it, even when they don't want process log at all.

[raymens]

Not sure if it's relevant but we use | Success<'TResult, 'DomainMessage> | Failure<'DomainMessage list> and are watching Chessie to see if we should switch to it instead of maintaining our own (based on Scott Wlaschins demo implementation with some additional stuff added).

We use the message with the Success (Ok in Chessie's case I think) regularly and having a 3rd Union case would probably be somewhat overkill to take care of.

Just my two cents.

[pblasucci]

I've been noodling this a bit. What about an approach that mostly matches Scott Wlaschin's with some niceties overlaid? Specifically, a core union type:

type Outcome<'success,'message> =
  | Pass of success:'success * warning:'message list
  | Fail of failure:'message list

plus many of the functions Scott talks about here. For example:

let either withPass withFail outcome =
    match outcome with
    | Pass (v,log) -> withPass (v,log) 
    | Fail failure -> withFail failure

Then, we layer in an Active Pattern for the simpler (no log) scenario:

let (|Ok|Bad|) outcome =
    match outcome with
    | Pass (v,_)  -> Ok v
    | Fail errors -> Bad errors

And also provide some functions which accommodate the absence of the warning log. These functions would likely follow some sort of naming convention. For example:

let eitherQuiet withPass withFail outcome = 
    let inline withOk (v,_) = withPass v
    either withOk withFail outcome

(Note: the suffix Quiet is used here for illustrative purposes only. We can surely come up with a better name.) Alternately, we can just ship two modules (one for each set of signatures). At any rate, I've got about 70% of this approach done (would send a PR at 100%). And I thought I'd see if there was any interest in continuing in this direction.

Feedback wanted.

[forki]

sounds good.

[pblasucci]

Please see #22