-
Notifications
You must be signed in to change notification settings - Fork 86
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 content for api response on upload SDK methods #2381
Conversation
Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2381 +/- ##
==========================================
- Coverage 91.28% 91.27% -0.01%
==========================================
Files 638 638
Lines 18146 18151 +5
Branches 3915 3917 +2
==========================================
+ Hits 16564 16568 +4
- Misses 1581 1582 +1
Partials 1 1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
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.
Thanks Jace for the enhancement - this LGTM!
I was curious to get your thoughts on setting the from
property in apiResponse
to include the bytes when its a buffer? You can use the Buffer.prototype.inspect
function to do so:
const buf = Buffer.from("test string").inspect();
// output: '<Buffer 74 65 73 74 20 73 74 72 69 6e 67>'
If we only want to show the first x bytes to prevent a massive string, you can use the subarray(0, x)
function on the buffer before calling inspect
. It's definitely not needed in the output, so I'm not requesting any changes here - but I figured it was worth mentioning, as the hardcoded strings Buffer<>
and Stream<>
appear to be inspired by this output.
(Note: this function does not exist for streams, but this suggestion mostly applies to buffers)
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.
LGTM! 😋
left a few minor comments, but the code (and tests LGTM!) 🙏
Signed-off-by: jace-roell <[email protected]>
Quality Gate failedFailed conditions |
Release succeeded for the The following packages have been published:
Powered by Octorelease 🚀 |
What It Does
Add content for api response on upload SDK methods
How to Test
Invoke one of the following SDK methods and view results.
Review Checklist
I certify that I have: