-
Notifications
You must be signed in to change notification settings - Fork 80
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
refactor: remove unnecessary new Promise
wrappers
#1110
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1110 +/- ##
==========================================
+ Coverage 75.54% 75.67% +0.12%
==========================================
Files 80 80
Lines 16164 16118 -46
Branches 1513 1507 -6
==========================================
- Hits 12211 12197 -14
+ Misses 3914 3882 -32
Partials 39 39
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked trough the changes and they seem all fine by me 👍
I would also argue after merging this PR we should close #569.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! regarding the issue linked, I think it might be difficult to be sure that every non-necessary promise it has been removed. I remember finding an eslint extension... maybe we could resolve it once the extension is in place and the non-convertible cases are correctly opted out.
Given we have two approvals I'm merging, let's wait for the eslint rule before closing #569 |
That sounds good :) I updated the PR description before your merge so that #569 will stay open. |
This PR contributes to resolving #569. There are a few instances of the
return Promise
pattern left that were a bit difficult to resolve, though, so I am not sure whether we should keep the issue open?