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

Implement error transformer for resultToPromise #46

Closed
wants to merge 1 commit into from
Closed

Implement error transformer for resultToPromise #46

wants to merge 1 commit into from

Conversation

seprich
Copy link
Contributor

@seprich seprich commented Dec 18, 2019

Previous change to the resultToPromise function throws the contents of errors away replacing it with meaningless FutureError. This PR fixes this bug, but is not backwards compatible because there is a new parameter errorTransformer.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 78

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 98.675%

Totals Coverage Status
Change from base Build 76: -0.03%
Covered Lines: 132
Relevant Lines: 133

💛 - Coveralls

Copy link
Collaborator

@scull7 scull7 left a comment

Choose a reason for hiding this comment

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

:shipit: I'm also on-board with these changes

@briangorman
Copy link
Collaborator

@seprich Could you implement this as an optional parameter with a default transformer keeping the current future error as the default? Ultimately I don't think many people are clamoring for this feature and the current behavior is "good enough" for most use cases, but I can understand some users wanting greater control

@seprich
Copy link
Contributor Author

seprich commented Jan 10, 2020

In fromPromise the errorTransformer is not an optional parameter, why should resultToPromise be any different? Also how is ignoring errors "good enough"? Before the recent fix of test library the tests may not have worked as expected and someone may have assumed that this function (resultToPromise) worked better than it did but in fact it just throws the error contents away. I'd consider that as a bug, not a feature. You are free to disagree of course.

@seprich seprich closed this Feb 9, 2020
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.

4 participants