-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: add delete endpoint (#345) #346
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Pratiksha Sankhe <[email protected]> Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Reviewer's Guide by SourceryThis pull request implements a new endpoint to delete DRS objects by their ID. The changes include adding a new File-Level Changes
Tips
|
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.
Hey @psankhe28 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider improving error handling in the deleteObject function. The current implementation returns different structures for errors vs. success, which could lead to inconsistent behavior. Also, consider checking and handling specific HTTP status codes.
- The JSDoc for the deleteObject function is less detailed compared to postObject. Please enhance the documentation to maintain consistency and provide more information about parameters and return values.
- No tests have been added for the new deleteObject endpoint. Given that this is a data-modifying operation, it's crucial to have comprehensive tests to ensure correct functionality and handle various scenarios.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 2 issues found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
* @param {object} objectData - The data of the drs object to be posted. | ||
* This should be an object containing the necessary fields for the drs object. | ||
* Modify the structure according to your object requirements. | ||
* @returns {string} - A string that denotes the object id |
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.
issue: Update JSDoc to match actual return type of postObject
The JSDoc indicates that postObject returns a string, but the implementation suggests it returns an object. Please update the documentation to accurately reflect the function's behavior.
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.
@psankhe28, please take a look at this
method: "DELETE", | ||
}); | ||
|
||
if (!response) { |
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.
consider some more robust error handling like checking the status text, as checking for just the response object won't catch many errors that may occur.
Also consider this for the other postObject
function.
@psankhe28, please address the request changes and resolve conflicts with main so I can re-review. |
1845262
to
1395942
Compare
Hey @psankhe28, can we have this merged? |
Description
It adds an endpoint to delete the drs object based on the object id
Fixes #(issue)
#333
Checklist
Comments
Summary by Sourcery
Add a delete endpoint to the API for removing DRS objects by ID and update the documentation to reflect this new functionality.
New Features:
Documentation: