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: turn conversion panics to errors #143

Merged
merged 2 commits into from
Jun 25, 2024
Merged

genai: turn conversion panics to errors #143

merged 2 commits into from
Jun 25, 2024

Conversation

jba
Copy link
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
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
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
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
2 checks passed
@jba jba deleted the jba-catch-pv branch June 25, 2024 16:56
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