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

surface mime type when passing attachments to user #1409

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

isahers1
Copy link
Contributor

No description provided.

@@ -2770,6 +2770,7 @@ export class Client implements LangSmithTracingClientInterface {
(acc, [key, value]) => {
acc[key.slice("attachment.".length)] = {
presigned_url: value.presigned_url,
mime_type: value.mime_type || undefined,
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need || undefined, just value.mime_type

@@ -40,6 +40,10 @@ test("evaluate can handle examples with attachments", async () => {
const expectedData = new Uint8Array(
Buffer.from("fake image data for testing")
);
const attachmentMimeType = config?.attachments?.["image"].mime_type;
Copy link
Collaborator

Choose a reason for hiding this comment

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

config?.attachments?.image.mime_type; is more conventional

For JS you only use box notation like that for things that have characters that can be represented inline e.g. if you were to do:

config?.attachments?.["foo-bar"].mime_type;

Copy link
Contributor

@agola11 agola11 left a comment

Choose a reason for hiding this comment

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

approving to unblock, but please address Jacob's comments

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