Skip to content

Conversation

nicksanford
Copy link
Member

No description provided.

@nicksanford nicksanford requested a review from a team as a code owner September 30, 2025 16:10
Copy link
Member

@stuqdog stuqdog left a 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!

* @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
Copy link
Member

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.

tags?: string[],
datasetIds?: string[]
) {
const fileExtension = fileName.includes('.') ? fileName.split('.')[1] : '';
Copy link
Member

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?

@njooma njooma requested a review from stuqdog September 30, 2025 21:45
Comment on lines +73 to +79
componentType?: string;

/** Optional name of the component associated with the file. */
componentName?: string;

/** Optional name of the method associated with the file. */
methodName?: string;
Copy link
Member Author

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

Copy link
Member

@stuqdog stuqdog left a 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)

* '123abc45-1234-5678-90ab-cdef12345678',
* '.jpg',
* [new Date('2025-03-19'), new Date('2025-03-19')]
* 'INSERT YOUR PART ID'
Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch

Copy link
Member Author

@nicksanford nicksanford left a 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?

Copy link
Member Author

@nicksanford nicksanford left a 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!

@njooma njooma merged commit b896a75 into viamrobotics:main Oct 1, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants