Skip to content
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 additional properties to FileMetadataResponse #147

Merged
merged 2 commits into from
May 22, 2024
Merged

Conversation

hsubox76
Copy link
Collaborator

Add fields error and videoMetadata to FileMetadataResponse.

See file.proto (internal link)

Tested on backend - output looks as expected.

@hsubox76 hsubox76 marked this pull request as ready for review May 21, 2024 22:25
@hsubox76 hsubox76 requested review from DellaBitta and dlarocque May 21, 2024 22:25
Copy link
Collaborator

@dlarocque dlarocque left a comment

Choose a reason for hiding this comment

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

Not specific to this PR, but I noticed that we populate most of the time and duration fields from RPC responses as strings. I feel like this would be very inconvenient for the user if they want to add any logic based on these responses. To make it easier for users to add logic based on responses, would we eventually want to encode these into a nicer interface (e.g. a Date for something like createTime)? Otherwise it seems like the user has to find a way to parse this string to be able to add some logic.

It look like Python does this (https://github.com/google-gemini/generative-ai-python/blob/main/google/generativeai/types/file_types.py#L53), but they seem to be using some other library to do this for them.. (maybe this)?

* Optional additional error details.
* @public
*/
export interface ErrorDetail {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we declare this is as ErrorDetails instead? It would align with the description in the comment as well as how it's declared in Firebase https://github.com/firebase/firebase-js-sdk/pull/8240/files#diff-77b1ce06b31ee6396f7b59b30e945f2fd334f86146a1f555a1b921537a453751R107

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess it was a grammar nit but the property this is assigned to is details (line 110) so it's details?: ErrorDetail[] where details is an array of ErrorDetails so I figured the repeating object should be singular. Although actually the implementation elsewhere in this same repo is ErrorDetails https://github.com/google-gemini/generative-ai-js/blob/main/packages/main/src/errors.ts#L70 so I'd rather not break it. I might even just import this type from there, it already has all fields optional and a [key: string]: unknown; so it should be compatible.

@hsubox76
Copy link
Collaborator Author

Not specific to this PR, but I noticed that we populate most of the time and duration fields from RPC responses as strings. I feel like this would be very inconvenient for the user if they want to add any logic based on these responses. To make it easier for users to add logic based on responses, would we eventually want to encode these into a nicer interface (e.g. a Date for something like createTime)? Otherwise it seems like the user has to find a way to parse this string to be able to add some logic.

It look like Python does this (https://github.com/google-gemini/generative-ai-python/blob/main/google/generativeai/types/file_types.py#L53), but they seem to be using some other library to do this for them.. (maybe this)?

They're using self.proto there, which is directly tapping into File.proto above - they're able to import and use the proto directly and convert it to a protobuf Timestamp string. Looking further into protobuf Timestamp it looks like the comments say using the JS Date object's toISOString() method will get you the required string format: https://source.corp.google.com/piper///depot/google3/google/protobuf/timestamp.proto;rcl=635929390;l=171 (internal link) So we could possibly add some helper methods. Although since this is a response field, and it's a standard ISO string, the user only has to wrap it in new Date() to get a Date out of it, so it isn't that much work for them, and they may want access to the raw string in some cases and it would be bad if we automatically wrapped it and they couldn't access it. Are there any cases where a field like this is an input and not a response? If so, wrapping it there would probably be helpful.

Formatting Duration might be a little more complicated: https://source.corp.google.com/piper///depot/google3/google/protobuf/duration.proto (internal link)

I don't know of a JS library that does that formatting (none are listed in the comment above) and I'd prefer not to add external deps to this project which curently has none.

@dlarocque dlarocque self-requested a review May 22, 2024 17:07
@hsubox76 hsubox76 merged commit ee02ff0 into main May 22, 2024
4 checks passed
@hsubox76 hsubox76 deleted the ch-file-props branch May 22, 2024 20:58
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.

3 participants