Skip to content

feat(go): use GenkitError instead of regular error #2643

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

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

sahdev77
Copy link
Contributor

@sahdev77 sahdev77 commented Apr 4, 2025

Addition of new Genkit error in go:

  • Handling Error using Genkit Error.
  • Removal of HTTPError from misc.
    Resolving : 2476
    Checklist (if applicable):
  • PR title is following https://www.conventionalcommits.org/en/v1.0.0/
  • Tested (manually, unit tested, etc.)
  • Docs updated (updated docs or a docs bug required)

@pavelgj pavelgj requested a review from apascal07 April 7, 2025 12:45
@sahdev77 sahdev77 changed the title Improve errors throughout feat(go): Handling of Errors in go with GenkitError Apr 7, 2025
@sahdev77 sahdev77 marked this pull request as ready for review April 8, 2025 14:49
@apascal07 apascal07 changed the title feat(go): Handling of Errors in go with GenkitError feat(go): use GenkitError instead of regular error Apr 15, 2025
Copy link
Collaborator

@apascal07 apascal07 left a comment

Choose a reason for hiding this comment

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

Can you modify a sample to return errors in various places and verify in the Dev UI that the errors look correct and have a useful stack? Conversely, the stack should not appear when calling the prod server, only the reflection server.


// Package status defines canonical status codes, names, and related types
// inspired by gRPC status codes.
package core
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are a lot of exported constants here that aren't qualified, it might make sense to put this in a error directory and package rather than pollute the core namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so are you suggesting to create a error package all together ?
As of now it is in sync with the structure that we had in Js and py but go behaves differently so we can think of keeping it as a separate package itself(genkitErr)

Copy link
Collaborator

Choose a reason for hiding this comment

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

JS and Python allow importing things piecemeal instead of the entire package like in Go so it's more important for things to be encapsulated well.

@sahdev77 sahdev77 requested a review from apascal07 April 16, 2025 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants