-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add deleteComment() API and support DELETE responses better #1115
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1115 +/- ##
==========================================
- Coverage 98.02% 97.98% -0.05%
==========================================
Files 59 59
Lines 2030 2038 +8
Branches 519 523 +4
==========================================
+ Hits 1990 1997 +7
- Misses 30 31 +1
Partials 10 10
Continue to review full report at Codecov.
|
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.
r+wc
src/api/index.tsx
Outdated
return callApi<{ | ||
requestData: undefined; | ||
successfulResponse: { ok: boolean }; | ||
}>({ |
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.
Shouldn't this be GetResource<{ok: boolean}>
?
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 guess it could be but it's not a GET
request. Also, I couldn't think of a better name for GetResource
. EmptyRequestResponse
seems weird, heh. Maybe I'll think of one.
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.
Maybe GetResource
could be renamed to Resource
?
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.
Hmm, seems sort of vague. I went with ResponseOnly
, let me know what you think.
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.
Works for me
Thanks for the quick review |
Fixes #1114