[RFC FS-1004] FSharpResult type with a few tests #964

Merged
merged 10 commits into from Jul 7, 2016

Conversation

Projects
None yet
9 participants
@wallymathieu
Contributor

wallymathieu commented Feb 12, 2016

This is the implementation of RFC FS-1004 Result type

@msftclas

This comment has been minimized.

Show comment
Hide comment
@msftclas

msftclas Feb 12, 2016

Hi @wallymathieu, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

Hi @wallymathieu, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@msftclas

This comment has been minimized.

Show comment
Hide comment
@msftclas

msftclas Feb 12, 2016

@wallymathieu, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@wallymathieu, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@msftclas msftclas added cla-signed and removed cla-required labels Feb 12, 2016

@enricosada

This comment has been minimized.

Show comment
Hide comment
@enricosada

enricosada Feb 14, 2016

Contributor

@wallymathieu can you add

Contributor

enricosada commented Feb 14, 2016

@wallymathieu can you add

@@ -601,7 +601,7 @@ namespace Microsoft.FSharp.Control
let mutable defaultCancellationTokenSource = new CancellationTokenSource()
[<NoEquality; NoComparison>]
- type Result<'T> =
+ type AsyncImplResult<'T> =

This comment has been minimized.

@forki

forki Feb 14, 2016

Contributor

Is this a public type?

@forki

forki Feb 14, 2016

Contributor

Is this a public type?

This comment has been minimized.

@wallymathieu

wallymathieu Feb 14, 2016

Contributor

This is a private type. It's not exposed from control.fsi.

@wallymathieu

wallymathieu Feb 14, 2016

Contributor

This is a private type. It's not exposed from control.fsi.

@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>

This comment has been minimized.

@enricosada

enricosada Feb 14, 2016

Contributor

to minimize diff revert this line

@enricosada

enricosada Feb 14, 2016

Contributor

to minimize diff revert this line

@@ -84,7 +84,7 @@
<Compile Include="FSharp.Core\Microsoft.FSharp.Collections\HashIdentityModule.fs" />
<Compile Include="FSharp.Core\Microsoft.FSharp.Collections\ListModule.fs" />
<Compile Include="FSharp.Core\Microsoft.FSharp.Collections\ListModule2.fs" />
- <Compile Include="FSharp.Core\Microsoft.FSharp.Collections\ObsoleteListFunctions.fs" />
+ <Compile Include="FSharp.Core\Microsoft.FSharp.Collections\ObsoleteListFunctions.fs" />

This comment has been minimized.

@enricosada

enricosada Feb 14, 2016

Contributor

to minimize diff revert this line

@enricosada

enricosada Feb 14, 2016

Contributor

to minimize diff revert this line

@@ -99,13 +99,14 @@
<Compile Include="FSharp.Core\Microsoft.FSharp.Collections\StringModule.fs" />
<Compile Include="FSharp.Core\PrimTypes.fs" />
<Compile Include="FSharp.Core\ComparersRegression.fs" />
- <Compile Include="FSharp.Core\DiscrimantedUnionType.fs" />
+ <Compile Include="FSharp.Core\DiscrimantedUnionType.fs" />

This comment has been minimized.

@enricosada

enricosada Feb 14, 2016

Contributor

to minimize diff revert this line

@enricosada

enricosada Feb 14, 2016

Contributor

to minimize diff revert this line

@@ -121,6 +122,6 @@
<CustomCopyLocal Include="FSharp.Core.Unittests.dll.config">
<TargetFilename>FSharp.Core.Unittests.dll.config</TargetFilename>
</CustomCopyLocal>
- </ItemGroup>
+ </ItemGroup>

This comment has been minimized.

@enricosada

enricosada Feb 14, 2016

Contributor

to minimize diff revert this line

@enricosada

enricosada Feb 14, 2016

Contributor

to minimize diff revert this line

<Import Project="$(FSharpSourcesRoot)\FSharpSource.targets" />
-</Project>
+</Project>

This comment has been minimized.

@enricosada

enricosada Feb 14, 2016

Contributor

to minimize diff revert this line

@enricosada

enricosada Feb 14, 2016

Contributor

to minimize diff revert this line

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Feb 14, 2016

Contributor

How does this relate to Chessie? Are you going to port the tests over? If we really put this into FSharp.Core then it should at least do what Chessie can. Especially in regards to C# interop this is a really hard topic. I tried so many things to make Chessie work for C# - so please also port the C# samples / tests to show that we have at elast that basic C# support that Chessie has.

Contributor

forki commented Feb 14, 2016

How does this relate to Chessie? Are you going to port the tests over? If we really put this into FSharp.Core then it should at least do what Chessie can. Especially in regards to C# interop this is a really hard topic. I tried so many things to make Chessie work for C# - so please also port the C# samples / tests to show that we have at elast that basic C# support that Chessie has.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Feb 14, 2016

Contributor
/// <summary>Helper type for error handling without exceptions.</summary>
[<StructuralEquality; StructuralComparison>]
[<CompiledName("FSharpResult`2")>]
type Result<'T1,'T2> = 
| Success of 'T1 
| Error of 'T2

Is that really the type we want for Results? Chessie is using:

/// Represents the result of a computation.
type Result<'TSuccess, 'TMessage> = 
/// Represents the result of a successful computation.
| Ok of 'TSuccess * 'TMessage list
/// Represents the result of a failed computation.
| Bad of 'TMessage list
Contributor

forki commented Feb 14, 2016

/// <summary>Helper type for error handling without exceptions.</summary>
[<StructuralEquality; StructuralComparison>]
[<CompiledName("FSharpResult`2")>]
type Result<'T1,'T2> = 
| Success of 'T1 
| Error of 'T2

Is that really the type we want for Results? Chessie is using:

/// Represents the result of a computation.
type Result<'TSuccess, 'TMessage> = 
/// Represents the result of a successful computation.
| Ok of 'TSuccess * 'TMessage list
/// Represents the result of a failed computation.
| Bad of 'TMessage list

@forki forki referenced this pull request in fsharp/fslang-design Feb 14, 2016

Closed

[RFC FS-1004 Discussion] Result type #49

@wallymathieu wallymathieu changed the title from FsharpResult type with a few tests to [WIP] FsharpResult type with a few tests Feb 14, 2016

@wallymathieu

This comment has been minimized.

Show comment
Hide comment
@wallymathieu

wallymathieu Feb 14, 2016

Contributor

@enricosada there, I've updated the description and title.

Contributor

wallymathieu commented Feb 14, 2016

@enricosada there, I've updated the description and title.

+// Copyright (c) Microsoft Corporation. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+// Various tests for:
+// Microsoft.FSharp.Core.ExtraTopLevelOperators.printf

This comment has been minimized.

@mexx

mexx Feb 17, 2016

Contributor

Please adjust this comment to match the test target.

@mexx

mexx Feb 17, 2016

Contributor

Please adjust this comment to match the test target.

@KevinRansom

This comment has been minimized.

Show comment
Hide comment
@KevinRansom

KevinRansom Jun 25, 2016

Contributor

@wallymathieu @forki @dsyme @otawfik-ms
Guys,
where are we on this PR? it hasn't been touched since mid-feb and so looks like it might be dead. Do we want to add this functionality, shall we find someone to shepherd it through or drop it?

Kevin

Contributor

KevinRansom commented Jun 25, 2016

@wallymathieu @forki @dsyme @otawfik-ms
Guys,
where are we on this PR? it hasn't been touched since mid-feb and so looks like it might be dead. Do we want to add this functionality, shall we find someone to shepherd it through or drop it?

Kevin

src/fsharp/FSharp.Core/prim-types.fsi
+ [<StructuralEquality; StructuralComparison>]
+ [<CompiledName("FSharpResult`2")>]
+ type Result<'T1,'T2> =
+ | Success of 'T1

This comment has been minimized.

@dsyme

dsyme Jun 25, 2016

Contributor

Please rename Success to Ok

@dsyme

dsyme Jun 25, 2016

Contributor

Please rename Success to Ok

This comment has been minimized.

@dsyme

dsyme Jun 25, 2016

Contributor

Please rename the type variables to T and TError

@dsyme

dsyme Jun 25, 2016

Contributor

Please rename the type variables to T and TError

src/fsharp/FSharp.Core/prim-types.fs
+ [<StructuralEquality; StructuralComparison>]
+ [<CompiledName("FSharpResult`2")>]
+ type Result<'T1,'T2> =
+ | Success of 'T1

This comment has been minimized.

@dsyme

dsyme Jun 25, 2016

Contributor

Please rename this to Ok

@dsyme

dsyme Jun 25, 2016

Contributor

Please rename this to Ok

src/fsharp/FSharp.Core/prim-types.fsi
+ type Result<'T1,'T2> =
+ | Success of 'T1
+ | Error of 'T2
+

This comment has been minimized.

@dsyme

dsyme Jun 25, 2016

Contributor

Please add /// comments to each union case

@dsyme

dsyme Jun 25, 2016

Contributor

Please add /// comments to each union case

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 25, 2016

Contributor

Please make the adjustments mentioned above and then we can accept this,

There is discussion in the RFC about whether we should also add a corresponding module of operations and a attempt builder into FSharp.Core as well. But minimally there is real value in accepting the definition of the type to help enable interop between different F# components that want to use this type.

Contributor

dsyme commented Jun 25, 2016

Please make the adjustments mentioned above and then we can accept this,

There is discussion in the RFC about whether we should also add a corresponding module of operations and a attempt builder into FSharp.Core as well. But minimally there is real value in accepting the definition of the type to help enable interop between different F# components that want to use this type.

@wallymathieu

This comment has been minimized.

Show comment
Hide comment
@wallymathieu

wallymathieu Jun 26, 2016

Contributor

Should I do a merge from master and fix the compilation error in Commands.fs?

Contributor

wallymathieu commented Jun 26, 2016

Should I do a merge from master and fix the compilation error in Commands.fs?

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 28, 2016

Contributor

@wallymathieu Yes please

Contributor

dsyme commented Jun 28, 2016

@wallymathieu Yes please

@wallymathieu

This comment has been minimized.

Show comment
Hide comment
@wallymathieu

wallymathieu Jul 3, 2016

Contributor

The failure of Windows_NT Release_ci_part2 Build :
FSharp.Core.Unittests.FSharp_Core.Microsoft_FSharp_Control.AsyncModule.Async.AwaitWaitHandle does not leak memory
Does not look related to this pull request.

Contributor

wallymathieu commented Jul 3, 2016

The failure of Windows_NT Release_ci_part2 Build :
FSharp.Core.Unittests.FSharp_Core.Microsoft_FSharp_Control.AsyncModule.Async.AwaitWaitHandle does not leak memory
Does not look related to this pull request.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jul 3, 2016

Contributor

CLose/reopen to re-run CI

Contributor

dsyme commented Jul 3, 2016

CLose/reopen to re-run CI

@dsyme dsyme closed this Jul 3, 2016

@dsyme dsyme reopened this Jul 3, 2016

@msftclas

This comment has been minimized.

Show comment
Hide comment
@msftclas

msftclas Jul 3, 2016

Hi @wallymathieu, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

msftclas commented Jul 3, 2016

Hi @wallymathieu, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@wallymathieu

This comment has been minimized.

Show comment
Hide comment
@wallymathieu

wallymathieu Jul 6, 2016

Contributor

Should we change the title and remove [WIP] ?

Contributor

wallymathieu commented Jul 6, 2016

Should we change the title and remove [WIP] ?

src/fsharp/FSharp.Core/prim-types.fs
@@ -3470,6 +3470,12 @@ namespace Microsoft.FSharp.Core
and 'T option = Option<'T>
+ [<StructuralEquality; StructuralComparison>]
+ [<CompiledName("FSharpResult`2")>]
+ type Result<'TOk,'TError> =

This comment has been minimized.

@dsyme

dsyme Jul 6, 2016

Contributor

Please change to just "T" instead of "TOk"

@dsyme

dsyme Jul 6, 2016

Contributor

Please change to just "T" instead of "TOk"

src/fsharp/FSharp.Core/prim-types.fsi
+ /// <summary>Helper type for error handling without exceptions.</summary>
+ [<StructuralEquality; StructuralComparison>]
+ [<CompiledName("FSharpResult`2")>]
+ type Result<'TOk,'TError> =

This comment has been minimized.

@dsyme

dsyme Jul 6, 2016

Contributor

Likewise here

@dsyme

dsyme Jul 6, 2016

Contributor

Likewise here

@dsyme dsyme changed the title from [WIP] FsharpResult type with a few tests to FSharpResult type with a few tests Jul 6, 2016

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jul 6, 2016

Contributor

@wallymathieu removed, thanks. Added one comment above TOk --> T

Contributor

dsyme commented Jul 6, 2016

@wallymathieu removed, thanks. Added one comment above TOk --> T

@dsyme dsyme changed the title from FSharpResult type with a few tests to [RFC FS-1004] FSharpResult type with a few tests Jul 6, 2016

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jul 6, 2016

Contributor

@KevinRansom @otawfik-ms This is ready to merge.

Contributor

dsyme commented Jul 6, 2016

@KevinRansom @otawfik-ms This is ready to merge.

@OmarTawfik

This comment has been minimized.

Show comment
Hide comment
@OmarTawfik

OmarTawfik Jul 6, 2016

Member

LGTM 👍

Member

OmarTawfik commented Jul 6, 2016

LGTM 👍

@OmarTawfik OmarTawfik merged commit aae9904 into Microsoft:master Jul 7, 2016

4 checks passed

Windows_NT Debug Build Build finished.
Details
Windows_NT Release_ci_part1 Build Build finished.
Details
Windows_NT Release_ci_part2 Build Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@cartermp

This comment has been minimized.

Show comment
Hide comment
@cartermp

cartermp Jul 7, 2016

Contributor

👍

Contributor

cartermp commented Jul 7, 2016

👍

@wallymathieu wallymathieu deleted the wallymathieu:fsharp_core_result branch Jul 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment