-
Notifications
You must be signed in to change notification settings - Fork 54
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
move allFutureThrowing
helper to tests
#1037
Conversation
We use a generically seeming helper to help us track three `Future` exceptions throughout the entire codebase. Move the helper to tests where it finds more use. The helper has several design flaws that make it incompatible with `{.async: (raises).}` usage, and has the potential to swallow one of the three exceptions that we track with it. The semantics should not change in this PR, this is a pure refactoring.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1037 +/- ##
============================================
+ Coverage 82.75% 82.77% +0.01%
============================================
Files 91 91
Lines 15616 15604 -12
============================================
- Hits 12923 12916 -7
+ Misses 2693 2688 -5
|
this PR I guess is fine:ish but it does introduce a bit of unnecessary churn - it would be more clear if first, one annotated all |
Yeah, doing those next, but the problem is So, getting rid of that problem first allows doing the |
We use a generically seeming helper to help us track three
Future
exceptions throughout the entire codebase. Move the helper to tests where it finds more use. The helper has several design flaws that make it incompatible with{.async: (raises).}
usage, and has the potential to swallow one of the three exceptions that we track with it.The semantics should not change in this PR, this is a pure refactoring.