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

genai: generalize uploadFile in examples and add comment #185

Merged
merged 4 commits into from
Jul 19, 2024
Merged

Conversation

eliben
Copy link
Member

@eliben eliben commented Jul 19, 2024

Updates #180

@eliben
Copy link
Member Author

eliben commented Jul 19, 2024

@fredsa

Refactored uploadFile based on your suggestion

genai/example_test.go Outdated Show resolved Hide resolved
@eliben eliben merged commit ebd7b02 into main Jul 19, 2024
2 checks passed
@eliben eliben deleted the upload-mime branch July 19, 2024 18:51
// representing it.
// representing it. mimeType optionally specifies the MIME type of the data in
// the file; if set to "", the service will try to automatically determine the
// type from the data contents.
// To clean up the file, defer a client.DeleteFile(ctx, file.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, and in the equivalent comment in docs-snippets-test.go, the word "try" suggests that the service may fail. It would be more accurate to say "it will try to determine the correct type"

Thinking about the comment a bit more, and trying to add some guidance and insight around when to specify the empty strings and when to provide a MIME type, how about something like:

mimeType optionally specifies the MIME type of the data in the file;
if omitted, the service will attempt to infer it from the file contents;
for ambiguous file types (e.g., text files), it is recommended to explicitly specify the MIME type

Copy link
Contributor

Choose a reason for hiding this comment

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

^ Feedback for future consideration

Copy link
Member Author

Choose a reason for hiding this comment

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

I like this phrasing; will send a PR soon

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