Both Result and Choice for error handling by wallymathieu · Pull Request #32 · jack-pappas/ExtCore

wallymathieu · 2017-10-30T19:34:41Z

Forked individual files from Using Result<,> instead of Choice<_,'Error> #31 and reverted original files
Added Compatibility namespaces and moved some Choice constructs into these
Renamed Notification.Error to Notification.Exception in order to avoid confusing things with Result<,>

wallymathieu · 2017-10-30T19:49:56Z

Note that I've added same kind of test as can be found for Fold for FoldBack @jack-pappas

wallymathieu · 2017-10-30T20:01:38Z

@vasily-kirichenko and @jack-pappas thoughts? Does it look ok?

vasily-kirichenko · 2017-10-31T14:25:03Z

ExtCore.Tests/Control.Result.fs

+open System.Runtime.CompilerServices
+open ExtCore.Control
+open NUnit.Framework
+//open FsCheck


vasily-kirichenko · 2017-10-31T14:25:53Z

ExtCore.Tests/Control.Result.fs

+            assertEqual (Ok true) result
+
+    /// <summary>Tests for <see cref="ExtCore.Control.ResultBuilder.While"/>.</summary>
+    module ``ResultBuilder_While`` =


Remove <summary> tag.

it's a bit extra, that's why?

vasily-kirichenko · 2017-10-31T14:27:53Z

ExtCore/Control.Cps.Result.fs

+
+
+/// <summary>
+/// </summary>


vasily-kirichenko · 2017-10-31T14:28:15Z

ExtCore/Control.Cps.Result.fs

+
+
+/// <summary>
+/// </summary>


vasily-kirichenko · 2017-10-31T14:45:51Z

ExtCore/Control.Cps.Result.fs

+/// <summary>
+/// </summary>
+[<Sealed>]
+type ProtectedResultStateContBuilder () =


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.

Indeed, should we move the "choice" versions to "compatibility" namespace, so that the result versions can have the same name ?

vasily-kirichenko · 2017-10-31T14:46:25Z

ExtCore/Control.Cps.Result.fs

+
+
+/// <summary>
+/// </summary>


vasily-kirichenko · 2017-10-31T14:47:05Z

ExtCore/Control.Cps.Result.fs

+[<AutoOpen>]
+module ResultWorkflowBuilders =
+
+    //


I don't see any value in all these empty comments. Maybe remove all of them everywhere?

vasily-kirichenko · 2017-10-31T14:49:06Z

ExtCore/Control.Indexed.Compability.fs

+open ExtCore
+open ExtCore.Control.Indexed
+/// Indexed-state workflows.
+[<RequireQualifiedAccess; CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>]


CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix) is not needed in most cases in F# 4.1, maybe remove all of them while you are at it?

https://fslang.uservoice.com/forums/245727-f-language/suggestions/14533251-make-modulesuffix-the-default-if-a-type-has-the

So yes, should be removed

vasily-kirichenko · 2017-10-31T14:49:42Z

ExtCore/Control.Indexed.Result.fs

+/// <typeparam name="S1"></typeparam>
+/// <typeparam name="S2"></typeparam>
+/// <typeparam name="T"></typeparam>
+/// <typeparam name="Error"></typeparam>


Remove all such meaningless XML Docs?

Could they have been added in order to silence warnings about missing xml docs?

maybe. if so, it smells like try..with _ -> ()

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

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.

jack-pappas · 2017-11-02T12:23:45Z

Thanks @wallymathieu — I’ll take a look at this over the weekend.

vasily-kirichenko · 2017-11-02T12:31:20Z

@jack-pappas could you please publish a new NuGet package after merging this PR?

wallymathieu · 2017-11-04T22:02:33Z

https://gist.github.com/wallymathieu/ad8fe3d5b85875cf6f487ad0180be660 ?

wallymathieu · 2017-11-11T17:49:36Z

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?

jack-pappas · 2017-11-12T20:49:04Z

ExtCore/Pervasive.fs


 /// Additional functional operators on options.
-[<RequireQualifiedAccess; CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>]
+[<RequireQualifiedAccess>]


Why was the CompilationRepresentation attribute removed here?

I think it was due to a thing @vasily-kirichenko said, that it's no longer needed.

vasily-kirichenko · 2017-12-25T07:34:50Z

Is there a plan to merge this?

wallymathieu · 2018-01-27T07:40:22Z

Should we close this pull request @jack-pappas (as it is to big) ?

wallymathieu · 2018-03-19T15:03:06Z

I'm closing this PR as it is probably to big to review or accept. Best of luck!

vasily-kirichenko · 2018-07-15T17:46:51Z

@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.

wallymathieu · 2018-11-02T10:48:12Z

Since @vasily-kirichenko says that it's working fine, I've reopened the pull request

vasily-kirichenko · 2018-11-02T10:55:24Z

Yes, it's working 100% fine.

wallymathieu · 2018-11-09T07:43:16Z

Then perhaps move this PR to point to another branch on this repository in order to iterate on it?

vasily-kirichenko · 2018-11-09T07:52:30Z

@wallymathieu Could you merge master into it to make AppVeyor happy?

wallymathieu · 2018-11-09T07:55:17Z

Will do!

…oice_for_errors

wallymathieu added 3 commits October 30, 2017 20:33

First take on dual Result and Choice version of ExtCore

145e437

Renaming Notification.Error to Notification.Exception

1b0028c

Using same sort of test for FoldBack as can be found for Fold

4a17699

Moving ProtectedState with Choice into Compability

4163435

vasily-kirichenko reviewed Oct 31, 2017

View reviewed changes

wallymathieu added 4 commits October 31, 2017 20:08

Moved conflicting Choice to Compatibility

c8caf70

Cleanup: Accidentally duplicated more than intended

57b6c51

Moved Choice methods to Compatibility namespace

85110bb

Compilation due to Choice moved to Compatibility

b646aa3

jack-pappas reviewed Nov 12, 2017

View reviewed changes

wallymathieu and others added 4 commits November 13, 2017 20:59

Merge branch 'master' into using_result_and_choice_for_errors

30888d3

Adding comment to result continuation func

90c3fbe

Reverting change to get smaller diff

aece156

Normalising cmd pack with rest of cmd in appveyor yml

006bdcf

wallymathieu closed this Mar 19, 2018

vasily-kirichenko mentioned this pull request Apr 18, 2018

Moving to fsprojects organization #41

Closed

jack-pappas mentioned this pull request Jul 15, 2018

Supporting multiple versions of FSharp.Core #44

Open

wallymathieu mentioned this pull request Nov 1, 2018

"Break early" from monad CE fsprojects/FSharpPlus#135

Closed

wallymathieu reopened this Nov 1, 2018

Merge remote-tracking branch 'origin/master' into using_result_and_ch…

2a83652

…oice_for_errors

wallymathieu requested review from jack-pappas and vasily-kirichenko January 16, 2020 08:14

vasily-kirichenko approved these changes Jan 16, 2020

View reviewed changes

Conversation

wallymathieu commented Oct 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wallymathieu commented Oct 30, 2017

Uh oh!

wallymathieu commented Oct 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jack-pappas commented Nov 2, 2017

Uh oh!

vasily-kirichenko commented Nov 2, 2017

Uh oh!

wallymathieu commented Nov 4, 2017

Uh oh!

wallymathieu commented Nov 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasily-kirichenko commented Dec 25, 2017

Uh oh!

wallymathieu commented Jan 27, 2018

Uh oh!

wallymathieu commented Mar 19, 2018

Uh oh!

vasily-kirichenko commented Jul 15, 2018

Uh oh!

wallymathieu commented Nov 2, 2018

Uh oh!

vasily-kirichenko commented Nov 2, 2018

Uh oh!

wallymathieu commented Nov 9, 2018

Uh oh!

vasily-kirichenko commented Nov 9, 2018

Uh oh!

wallymathieu commented Nov 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wallymathieu commented Oct 30, 2017 •

edited

Loading