Both Result and Choice for error handling #32
Conversation
|
Note that I've added same kind of test as can be found for Fold for FoldBack @jack-pappas |
|
@vasily-kirichenko and @jack-pappas thoughts? Does it look ok? |
ExtCore.Tests/Control.Result.fs
Outdated
| open System.Runtime.CompilerServices | ||
| open ExtCore.Control | ||
| open NUnit.Framework | ||
| //open FsCheck |
| assertEqual (Ok true) result | ||
|
|
||
| /// <summary>Tests for <see cref="ExtCore.Control.ResultBuilder.While"/>.</summary> | ||
| module ``ResultBuilder_While`` = |
There was a problem hiding this comment.
Remove <summary> tag.
There was a problem hiding this comment.
it's a bit extra, that's why?
|
|
||
|
|
||
| /// <summary> | ||
| /// </summary> |
|
|
||
|
|
||
| /// <summary> | ||
| /// </summary> |
ExtCore/Control.Cps.Result.fs
Outdated
| /// <summary> | ||
| /// </summary> | ||
| [<Sealed>] | ||
| type ProtectedResultStateContBuilder () = |
There was a problem hiding this comment.
I don't like the name. The "choice" version is named just "ProtectedState..." where "protected" means Either-style error handling. Here we have a kind of duplication: "protected" and "result" both point to the same aspect of the builder - errors handling.
There was a problem hiding this comment.
Indeed, should we move the "choice" versions to "compatibility" namespace, so that the result versions can have the same name ?
ExtCore/Control.Cps.Result.fs
Outdated
|
|
||
|
|
||
| /// <summary> | ||
| /// </summary> |
| [<AutoOpen>] | ||
| module ResultWorkflowBuilders = | ||
|
|
||
| // |
There was a problem hiding this comment.
I don't see any value in all these empty comments. Maybe remove all of them everywhere?
| open ExtCore | ||
| open ExtCore.Control.Indexed | ||
| /// Indexed-state workflows. | ||
| [<RequireQualifiedAccess; CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>] |
There was a problem hiding this comment.
CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix) is not needed in most cases in F# 4.1, maybe remove all of them while you are at it?
There was a problem hiding this comment.
There was a problem hiding this comment.
So yes, should be removed
| /// <typeparam name="S1"></typeparam> | ||
| /// <typeparam name="S2"></typeparam> | ||
| /// <typeparam name="T"></typeparam> | ||
| /// <typeparam name="Error"></typeparam> |
There was a problem hiding this comment.
Remove all such meaningless XML Docs?
There was a problem hiding this comment.
Could they have been added in order to silence warnings about missing xml docs?
There was a problem hiding this comment.
maybe. if so, it smells like try..with _ -> ()
There was a problem hiding this comment.
some of these things could be improved by a little bit of documentation, for instance It's a bit confusing to read the CPS function definitions
There was a problem hiding this comment.
I added these empty XML docs to remind myself -- years ago -- to fill them in so new users would get some helpful hints from Intellisense. Obviously, I still haven't gotten around to doing it, but I think Oskar's right -- it'd be helpful to users to have the information rather than just removing the comments.
|
Thanks @wallymathieu — I’ll take a look at this over the weekend. |
|
@jack-pappas could you please publish a new NuGet package after merging this PR? |
|
Should we perhaps try to split this pull request into more granular parts? A lot changes are mostly copy and modify (in order to be compatible). @vasily-kirichenko do you have an idea of how this could be done? |
ExtCore/Pervasive.fs
Outdated
| @@ -806,7 +791,7 @@ module Lazy = | |||
| #endif | |||
|
|
|||
| /// Additional functional operators on options. | |||
| [<RequireQualifiedAccess; CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>] | |||
| [<RequireQualifiedAccess>] | |||
There was a problem hiding this comment.
Why was the CompilationRepresentation attribute removed here?
There was a problem hiding this comment.
I think it was due to a thing @vasily-kirichenko said, that it's no longer needed.
|
Is there a plan to merge this? |
|
Should we close this pull request @jack-pappas (as it is to big) ? |
|
I'm closing this PR as it is probably to big to review or accept. Best of luck! |
|
@jack-pappas Please, merge this PR. I had to build an ExtCore package myself and publish it on a private server, everything's been working OK. |
|
Since @vasily-kirichenko says that it's working fine, I've reopened the pull request |
|
Yes, it's working 100% fine. |
|
Then perhaps move this PR to point to another branch on this repository in order to iterate on it? |
|
@wallymathieu Could you merge master into it to make AppVeyor happy? |
|
Will do! |
Compatibilitynamespaces and moved someChoiceconstructs into theseNotification.ErrortoNotification.Exceptionin order to avoid confusing things withResult<,>