[Feat] Machine-friendly exit codes for error classification#59
Conversation
Signed-off-by: samzong <[email protected]>
There was a problem hiding this comment.
Code Review
This pull request introduces a centralized exit code management system by adding an internal exitcode package and updating error handling in the git, llm, and cmd packages. The review feedback identifies a potential compilation issue on Go versions earlier than 1.20 due to the use of multiple error-wrapping verbs and suggests wrapping unclassified errors in the tag command with userFacingError to maintain consistent formatting across the CLI.
|
|
||
| if err != nil { | ||
| return "", fmt.Errorf("failed to call LLM: %w", err) | ||
| return "", fmt.Errorf("failed to call LLM: %w (%w)", err, ErrLLM) |
There was a problem hiding this comment.
The use of multiple %w format verbs for error wrapping was introduced in Go 1.20. If this project needs to maintain compatibility with older Go versions, this will cause a compilation error.
For compatibility with Go versions < 1.20, you could wrap only ErrLLM and include the original error's text in the message. This preserves the ability to check for ErrLLM while keeping the context.
For example:
return "", fmt.Errorf("failed to call LLM: %v: %w", err, ErrLLM)This would mean errors.As would no longer work for the original err, but errors.Is for ErrLLM would still function correctly.
|
|
||
| if err != nil { | ||
| return "", "", fmt.Errorf("failed to call LLM: %w", err) | ||
| return "", "", fmt.Errorf("failed to call LLM: %w (%w)", err, ErrLLM) |
There was a problem hiding this comment.
| if coded := classifyError(err); coded != nil { | ||
| return coded | ||
| } | ||
| return err |
There was a problem hiding this comment.
To ensure consistent error message formatting across all commands, unclassified errors from the tag command should also be wrapped in userFacingError, similar to how the root command handles them. This will provide a uniform experience for users, with errors prefixed by gmc: .
After making this change, please also ensure all error return paths in runTagCommand use this wrapper for consistency (e.g., the one at line 70 seems to have been missed).
| return err | |
| return userFacingError{msg: fmt.Sprintf("gmc: %v", err), err: err} |
Signed-off-by: samzong <[email protected]>
Signed-off-by: samzong <[email protected]>
What's changed?
internal/exitcodepackage defining typed exit codes:NoStagedChanges=10,NotGitRepo=11,LLMError=12main.goextractsexitcode.Errorviaerrors.Asbefore falling back toos.Exit(1)git.ErrNotGitRepoandllm.ErrLLMenableerrors.IsclassificationclassifyError()incmdpackage eliminates duplication between root and tag commandsWhy
Closes #55