Skip to content

genai: turn conversion panics to errors#143

Merged
jba merged 2 commits intomainfrom
jba-catch-pv
Jun 25, 2024
Merged

genai: turn conversion panics to errors#143
jba merged 2 commits intomainfrom
jba-catch-pv

Conversation

@jba
Copy link
Copy Markdown
Collaborator

@jba jba commented Jun 23, 2024

Return an error if a conversion fails, instead of panicking.

Currently the only possible such failure is when a FunctionResponse
contains a value that can't be converted to a Struct proto.

@jba jba requested a review from eliben June 23, 2024 14:48
Base automatically changed from jba-use-new-pv to main June 23, 2024 17:43
Return an error if a conversion fails, instead of panicking.

Currently the only possible such failure is when a FunctionResponse
contains a value that can't be converted to a Struct proto.
Copy link
Copy Markdown
Member

@eliben eliben left a comment

Choose a reason for hiding this comment

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

Two questions here:

  1. Shouldn't most of this be shared with vertex? I'm surprised to see catchPVPanic here and not in shared code
  2. The panic/error flow should be better documented with a comment block somewhere logical that explains what's going on

@jba
Copy link
Copy Markdown
Collaborator Author

jba commented Jun 24, 2024

I'll move the catch function to the generated code and also generate a comment for pvPanic.

@jba
Copy link
Copy Markdown
Collaborator Author

jba commented Jun 24, 2024

Done.

@jba jba requested a review from eliben June 24, 2024 14:11
@jba jba merged commit 5220ad2 into main Jun 25, 2024
@jba jba deleted the jba-catch-pv branch June 25, 2024 16:56
kasugamirai pushed a commit to kasugamirai/generative-ai-go that referenced this pull request Nov 11, 2025
… Pkl version (google#143)

Also: add a trailing newline to `codegen/src/PklProject.deps.json`.
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.

2 participants