Update function to handle multi-line commands for timestamps#5241
Update function to handle multi-line commands for timestamps#5241agrawalkhushi18 wants to merge 1 commit intoGoogleCloudPlatform:developfrom
Conversation
Summary of ChangesHello @agrawalkhushi18, 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 refines the timestamping behavior within the shell writer to significantly improve the readability of multi-line process logs. By introducing a character-based check, the system now intelligently avoids prepending timestamps to lines that are indented or empty, thereby preserving the original visual structure of complex command outputs, such as those from Highlights
Changelog
Activity
Using Gemini Code AssistThe 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 by creating a comment using either
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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses an issue where timestamps were incorrectly injected into indented, multi-line log outputs, which could disrupt formatting. The approach of checking the first character of a line segment for whitespace before adding a timestamp is sound, and the new test cases in pkg/shell/writer_test.go correctly validate this behavior. I have one suggestion to refactor the conditional logic in pkg/shell/writer.go to improve readability and maintainability.
| if len(p) > 0 { | ||
| firstChar := p[0] | ||
| // Avoid printing timestamp if the segment starts with an indentation or newline | ||
| if firstChar != ' ' && firstChar != '\t' && firstChar != '\n' { | ||
| ts := time.Now().UTC().Format(time.RFC3339) | ||
| coloredTs := logging.TsColor.Sprint(ts) | ||
| buf.WriteString(coloredTs + " ") | ||
| } | ||
| } |
There was a problem hiding this comment.
For better readability and maintainability, consider using a switch statement to check for leading whitespace characters. This makes the logic clearer than a chain of && conditions and is more easily extensible if you need to handle more character types in the future.
if len(p) > 0 {
switch p[0] {
case ' ', '\t', '\n':
// Do not add timestamp for indented or empty lines.
default:
ts := time.Now().UTC().Format(time.RFC3339)
coloredTs := logging.TsColor.Sprint(ts)
buf.WriteString(coloredTs + " ")
}
}References
- The style guide emphasizes that code should be maintainable and easy to understand (line 6). Using a
switchstatement improves readability and maintainability over a complexifcondition, which aligns with Go best practices mentioned in the style guide (line 27). (link)
This PR replaces timestamping in pkg/shell/writer.go with a character check (' ', '\t', '\n') to prevent injecting timestamps into indented multi-line process logs (like gcloud commands), preserving their original visual layout and readability. Updated
pkg/shell/writer_test.go to assert this new formatting behavior cleanly handles whitespace-prefixed segments without adding extra timestamps.
Submission Checklist
NOTE: Community submissions can take up to 2 weeks to be reviewed.
Please take the following actions before submitting this pull request.