Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Borrows more Cvdm code to enhance computation expressions #12

Merged
merged 6 commits into from Apr 10, 2019

Conversation

TheAngryByrd
Copy link
Collaborator

More work as part of #3.

This enhances the computation expressions to be on par with Cvdm

@cmeeren
Copy link
Contributor

cmeeren commented Mar 24, 2019

🙌

@demystifyfp
Copy link
Owner

Thanks, @TheAngryByrd for the contribution.
It'd be also helpful if you can add the associated tests as well. Thanks in advance :)

@cmeeren does it cover all the CE from Cvdm?

@cmeeren
Copy link
Contributor

cmeeren commented Mar 27, 2019

The best way to know is to also include all the tests and see if they pass. :)

That said, ResultBuilder.Combine is missing (so some of the tests probably wouldn't even compile). And some methods delegate to relevant module functions already existing in this library, which they should, but I can't immediately say whether it's exactly the same.

Type annotations should probably be explicitly specified, because some places they do impact the behaviour. I vaguely remember something about AsyncResult.Combine allowing stuff that didn't make sense for that CE until I locked it down with annotations that prevented such stuff from compiling (though I might misremember whether it was Combine or another method). Also, I am generally in favour of explicitly specifying type annotations in published libraries to ensure generic type params have descriptive/idiomatic names and are easier to read. But most important is the first point - missing type annotations in CEs can cause unexpected behaviour at runtime (unless you're a CE expert and know exactly what you're getting; I'm not, and the tests are the only way I am sure it works).

In summary, my recommendation is:

  • Import all CE members as-is from Cvdm.ErrorHandling, including type annotations
  • Import all CE tests
  • Optionally rewrite some members using functions already existing in FsToolkit.ErrorHandling, making sure tests still pass

@TheAngryByrd
Copy link
Collaborator Author

Sounds like a plan. I'll get to it this week.

@demystifyfp
Copy link
Owner

Thanks, @cmeeren for the detailed writeup and @TheAngryByrd for taking this up.

@demystifyfp
Copy link
Owner

demystifyfp commented Apr 6, 2019

Thanks, @TheAngryByrd for the changes. Is anything else pending? Can I release a beta version to test things out?

@cmeeren Are you available test this new one after I release the new version?

@cmeeren
Copy link
Contributor

cmeeren commented Apr 6, 2019

Sure, but if you want I can check out the PR branch and test before you merge/release. (With "test", I mean run the suite of unit tests in Cvdm.ErrorHandling.)

I'll try to do it today.

@cmeeren
Copy link
Contributor

cmeeren commented Apr 6, 2019

Good to go; all existing Cvdm.ErrorHandling tests pass using TheAngryByrd:more-CE-binds.

(Except the requireEqual and requireEqualTo tests, because these haven't been merged yet, see #3 (comment))

@demystifyfp
Copy link
Owner

Thanks, @cmeeren for testing these changes. @TheAngryByrd Can I merge it?

@TheAngryByrd
Copy link
Collaborator Author

Merge it! 👍

@cmeeren
Copy link
Contributor

cmeeren commented Apr 9, 2019

:shipit:

@demystifyfp demystifyfp merged commit e71d45e into demystifyfp:master Apr 10, 2019
@demystifyfp
Copy link
Owner

@TheAngryByrd @cmeeren I am facing some weird issues with the FSharp 4.6 update on my machine, and I am unable to publish the package because of this. I will release the new version as soon as I resolved this.

@TheAngryByrd TheAngryByrd deleted the more-CE-binds branch January 11, 2022 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants