Validation.Result uses Result instead of Choice #440
Conversation
|
Thanks for contributing! I skimmed through it and it looks OK. I will enable the CI run now and do a detailed check this week. |
| namespace FSharpx | ||
|
|
||
| [<System.Obsolete("This module is deprecated. Use Validation.Result instead.")>] | ||
| module Validation = |
There was a problem hiding this comment.
Note: During review check if the obsolete warning works as expected (bugs old code users and leaves new code users alone).
There was a problem hiding this comment.
Checked, works as expected.
|
@fdornak The build failed due to usage of the obsolete code in csharpcompat. Can you check if it is possible to optout from this warning in those parts? |
Replace Validation active pattern usage in ChoiceTests
|
I have added #nowarn pragmas which should fix the build. However it seems that they have file-wide effect and I haven't found a way around it. I found quite recent PR which might add scoped pragmas feature in the future dotnet/fsharp#18049 |
| actions |> Choice.foldM folder startingPosition | ||
|
|
||
| match finalPosition with | ||
| | Validation.Success (x,y) -> |
There was a problem hiding this comment.
Why do we want to stop using the active pattern?
There was a problem hiding this comment.
Why do we want to stop using the active pattern?
Currently the active pattern is in old "Choice" Validation module and this place is the only one in ChoiceTests that uses Validation module. My reasoning was that instead of marking #nowarn the whole ChoiceTests file it can be replaced with Choice1Of2 and avoid the dependency altogether.
I am ok with either approach. We could also move the active pattern in Choice module, which personally I like the most and then It would make more sense to use in ChoiceTests.
Let me know what you think
|
Hi Filip, I went through the code and noticed that if you use Bear with me a moment longer while I do the check from the note :) Edit: There is also the map call. |
|
I like it and I'm willing to merge it. The only question I have is the one about the active pattern. |
|
@fdornak Thank you very much for the contribution! I will release a new version this week :) |
|
I will release it this week. |
My first PR here 😉
Created a new submodule Validation.Result which uses Result type instead of Choice and marked Validation as obsolete.
Any suggestions are welcome
Resolves #428