Austinamoruso/prcomments #42
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @austinamorusoyardstick, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request focuses on refining the process of handling pull request comments, primarily by enhancing logging visibility, improving the structure and content of prompts sent to the AI, and streamlining the underlying processing logic for better efficiency.
Highlights
- Logging Enhancements: Improved logging for comment processing, including more detailed messages for skipped comments, total comment counts, and clearer previews of prompts sent to the AI.
- Prompt Formatting: The
formatCommentAsPromptmethod now includes additional context such as the comment's index, the total number of comments, and details about accepted pieces for split comments, providing richer information to the AI. - Code Simplification: Removed unnecessary test message logic and refined the delay mechanism in sequential comment processing, ensuring delays only occur between comments and are more efficient.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the pull request comment processing logic. The changes improve logging by making it more descriptive, enhance the prompt format for better context, and simplify the overall flow by removing redundant code and delays. The code is cleaner and more efficient.
app/pr_processor.go
Outdated
|
|
||
| // Skip empty prompts (e.g., split comments with no accepted pieces) | ||
| if prompt == "" { | ||
| log.WarningLog.Printf("Skipping comment %d (empty prompt)", i+1) |
There was a problem hiding this comment.
Consider using log.DebugLog instead of log.WarningLog for skipping comments. Debug logs are typically used for information that is helpful during development but not important in production. This will reduce noise in the warning logs.1
Style Guide References
| log.WarningLog.Printf("Skipping comment %d (empty prompt)", i+1) | |
| log.DebugLog.Printf("Skipping comment %d (empty prompt)", i+1) |
Footnotes
app/pr_processor.go
Outdated
| log.WarningLog.Printf("Sending PR comment %d/%d to Claude", i+1, len(comments)) | ||
| promptPreview := prompt | ||
| if len(promptPreview) > 100 { | ||
| promptPreview = promptPreview[:100] + "..." | ||
| } | ||
| log.WarningLog.Printf("Prompt content: %s", promptPreview) | ||
|
|
||
| // First try sending a simple test message | ||
| testMsg := fmt.Sprintf("Processing PR comment %d from @%s", i+1, comment.Author) | ||
| if comment.IsSplit { | ||
| acceptedCount := len(comment.GetAcceptedPieces()) | ||
| testMsg += fmt.Sprintf(" (%d pieces selected)", acceptedCount) | ||
| } | ||
| if err := selected.SendPromptToAI(testMsg); err != nil { | ||
| log.ErrorLog.Printf("Failed to send test message to Claude: %v", err) | ||
| return fmt.Errorf("failed to send test message to Claude: %w", err) | ||
| } | ||
|
|
||
| // Short pause before sending the actual prompt | ||
| time.Sleep(500 * time.Millisecond) | ||
| log.WarningLog.Printf("Prompt preview: %s", promptPreview) |
There was a problem hiding this comment.
These log messages appear to be for informational purposes, but they are being logged using log.WarningLog. Using the Warning level for routine operational messages can create noise and make it harder to identify actual warnings in the logs. It would be more appropriate to use log.InfoLog for these kinds of messages.
| log.WarningLog.Printf("Sending PR comment %d/%d to Claude", i+1, len(comments)) | |
| promptPreview := prompt | |
| if len(promptPreview) > 100 { | |
| promptPreview = promptPreview[:100] + "..." | |
| } | |
| log.WarningLog.Printf("Prompt content: %s", promptPreview) | |
| // First try sending a simple test message | |
| testMsg := fmt.Sprintf("Processing PR comment %d from @%s", i+1, comment.Author) | |
| if comment.IsSplit { | |
| acceptedCount := len(comment.GetAcceptedPieces()) | |
| testMsg += fmt.Sprintf(" (%d pieces selected)", acceptedCount) | |
| } | |
| if err := selected.SendPromptToAI(testMsg); err != nil { | |
| log.ErrorLog.Printf("Failed to send test message to Claude: %v", err) | |
| return fmt.Errorf("failed to send test message to Claude: %w", err) | |
| } | |
| // Short pause before sending the actual prompt | |
| time.Sleep(500 * time.Millisecond) | |
| log.WarningLog.Printf("Prompt preview: %s", promptPreview) | |
| // Debug: log the prompt being sent | |
| log.InfoLog.Printf("Sending PR comment %d/%d to Claude", i+1, len(comments)) | |
| promptPreview := prompt | |
| if len(promptPreview) > 100 { | |
| promptPreview = promptPreview[:100] + "..." | |
| } | |
| log.InfoLog.Printf("Prompt preview: %s", promptPreview) |
app/pr_processor.go
Outdated
| return fmt.Errorf("failed to send comment %d to Claude: %w", i+1, err) | ||
| } | ||
|
|
||
| log.WarningLog.Printf("Successfully sent comment %d to Claude", i+1) |
There was a problem hiding this comment.
This log message indicates a successful operation, which is typically considered Info level, not Warning. Using log.InfoLog would be more semantically correct and would help distinguish routine messages from actual warnings.
| log.WarningLog.Printf("Successfully sent comment %d to Claude", i+1) | |
| log.InfoLog.Printf("Successfully sent comment %d to Claude", i+1) |
austinamorusoyardstick
force-pushed
the
austinamoruso/prcomments
branch
from
7379781 to
00d08cd
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the pull request comment processing. The logging is more descriptive, and the code has been simplified by removing the test message logic and redundant delays. The prompt formatting is also enhanced to provide more context to the AI.
I have a couple of suggestions to further improve maintainability and performance:
- Replace a magic number for the delay with a constant.
- Optimize string building by using
fmt.Fprintfwithstrings.Builder.
Overall, these are great improvements to the codebase.
| time.Sleep(3 * time.Second) | ||
| // Delay between comments to give Claude time to process | ||
| if i < len(comments)-1 { | ||
| time.Sleep(2 * time.Second) |
There was a problem hiding this comment.
The sleep duration 2 * time.Second is a hardcoded magic number. It's a good practice to define this as a constant, for example const delayBetweenComments = 2 * time.Second. This improves readability by giving the value a descriptive name and makes it easier to modify in the future.
| prompt.WriteString(fmt.Sprintf("Processing PR comment %d of %d from @%s", index, total, comment.Author)) | ||
| if comment.IsSplit { | ||
| acceptedCount := len(comment.GetAcceptedPieces()) | ||
| prompt.WriteString(fmt.Sprintf(" (%d pieces selected)", acceptedCount)) | ||
| } | ||
| prompt.WriteString("\n\n") |
There was a problem hiding this comment.
When building a string with strings.Builder, it's more efficient and idiomatic to use fmt.Fprintf instead of multiple WriteString calls with fmt.Sprintf. This avoids intermediate string allocations from fmt.Sprintf and can improve performance.
| prompt.WriteString(fmt.Sprintf("Processing PR comment %d of %d from @%s", index, total, comment.Author)) | |
| if comment.IsSplit { | |
| acceptedCount := len(comment.GetAcceptedPieces()) | |
| prompt.WriteString(fmt.Sprintf(" (%d pieces selected)", acceptedCount)) | |
| } | |
| prompt.WriteString("\n\n") | |
| fmt.Fprintf(&prompt, "Processing PR comment %d of %d from @%s", index, total, comment.Author) | |
| if comment.IsSplit { | |
| acceptedCount := len(comment.GetAcceptedPieces()) | |
| fmt.Fprintf(&prompt, " (%d pieces selected)", acceptedCount) | |
| } | |
| prompt.WriteString("\n\n") |
1 participant
This pull request refactors the processing of pull request comments to improve logging, streamline the flow, and enhance the prompt formatting. The most important changes include adding more descriptive logging, modifying the
formatCommentAsPromptmethod to include additional context, and simplifying the comment processing logic.Logging improvements:
Prompt formatting enhancements:
formatCommentAsPromptmethod to include the comment index, total number of comments, and additional context such as the number of accepted pieces for split comments. This provides more informative prompts for processing.Code simplification:
processCommentsSequentiallymethod, reducing complexity and improving efficiency.