-
Notifications
You must be signed in to change notification settings - Fork 48
CONSULT-1797-implement-FileUpload-on-the-typescript-SDK #642
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
CONSULT-1797-implement-FileUpload-on-the-typescript-SDK #642
Conversation
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.
A couple small things and we should make sure tests are passing (no need to rerequest review), otherwise lgtm!
src/app/data-client.ts
Outdated
* @param componentName The name of the component used to capture the data | ||
* @param methodName The name of the method used to capture the data. | ||
* @param fileExtension The file extension of binary data including the | ||
* @param filename The file extension of binary data including the |
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.
This is kind of confusing to me, since we're still in the comment saying we expect a file extension.
src/app/data-client.ts
Outdated
tags?: string[], | ||
datasetIds?: string[] | ||
) { | ||
const fileExtension = fileName.includes('.') ? fileName.split('.')[1] : ''; |
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 don't think anything in principle would prevent someone from having a file named, e.g., my-image_v0.2.jpg
, might be better to always grab the last entry from fileName.split
rather than the second one?
componentType?: string; | ||
|
||
/** Optional name of the component associated with the file. */ | ||
componentName?: string; | ||
|
||
/** Optional name of the method associated with the file. */ | ||
methodName?: string; |
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.
remove, these are invalid on arbitrary data uploads
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.
one minor comment, otherwise lgtm (modulo Nick's comments)
src/app/data-client.ts
Outdated
* '123abc45-1234-5678-90ab-cdef12345678', | ||
* '.jpg', | ||
* [new Date('2025-03-19'), new Date('2025-03-19')] | ||
* 'INSERT YOUR PART 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.
(minor) This is unclear to me. Is this supposed to be an arg? If so we need a comma and the partID goes after the binary data, not before.
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.
good catch
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.
Have you tried actually using this to upload data to a robot?
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, ty @njooma for implementing this!
No description provided.