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

Fix Using/TryFinally asyncValidation CE #271

Merged
merged 2 commits into from
Jul 14, 2024

Conversation

1eyewonder
Copy link
Contributor

Proposed Changes

Related to #270, I think we have some issues around our TryFinally/Using code. I have updated the CE to redirect calls to the necessary async functions to try to fix the related issues. I think the code was originally written this way because I was referencing the validation CE rather than also paying attention to things asyncResult was doing.

Types of changes

What types of changes does your code introduce to FsToolkit.ErrorHandling?
Put an x in the boxes that apply and remove ones that don't apply

  • Bugfix (non-breaking change which fixes an issue)

Checklist

  • Build and tests pass locally
  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have added necessary documentation (if appropriate)

Further comments

I am open to suggestions on additional unit tests we could potentially include to catch issues like this. I also added the #if !FABLE_COMPILER && NETSTANDARD2_1 conditional on a unit test since I noticed it was in asyncResult and not asyncValidation

@1eyewonder
Copy link
Contributor Author

Evidence unit test changes passed prior to the CE changes. Per offline discussion with @TheAngryByrd, I'll try getting some additional unit tests spun up to help catch regression errors

image
image

@1eyewonder
Copy link
Contributor Author

Testing evidence of unit test failing in asyncValidation and passing in asyncResult on master branch
image

Testing evidence of unit test passing in asyncValidation and passing in asyncResult on PR
image

I also added these unit tests to the other async CE's to help catch any regression bugs

@TheAngryByrd TheAngryByrd self-requested a review July 14, 2024 01:07
Copy link
Collaborator

@TheAngryByrd TheAngryByrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks for all the detailed work!

@TheAngryByrd TheAngryByrd merged commit ba53f4b into demystifyfp:master Jul 14, 2024
25 checks passed
@TheAngryByrd TheAngryByrd changed the title Updated asyncValidation CE Fix Using/TryFinally asyncValidation CE Jul 14, 2024
TheAngryByrd added a commit that referenced this pull request Jul 14, 2024
- [Added XML Comments](#268) Credits @1eyewonder
- [Fix Using/TryFinally asyncValidation CE](#271) Credits @1eyewonder
TheAngryByrd added a commit that referenced this pull request Jul 14, 2024
- [Added XML Comments](#268) Credits @1eyewonder
- [Fix Using/TryFinally asyncValidation CE](#271) Credits @1eyewonder
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.

2 participants