feat: Refactor output processing with go-jq-yamlformat #34
Conversation
Refactor the output processing logic to use the go-jq-yamlformat library. This change replaces the manual JSON/YAML encoding and jq filtering with the more robust and flexible pipeline provided by go-jq-yamlformat. Key changes: - Replaced direct gojq calls with the jqyaml pipeline. - Introduced a `toProtojsonObject` function to reliably convert spannerpb.ResultSet to a jq-compatible format. - Reworked the handling of `--compact-output` and `--raw-output` flags to work correctly with the new library. - Added comprehensive unit tests for the new output processing logic. feat: Refactor output processing with go-jq-yamlformat Refactor the output processing logic to use the go-jq-yamlformat library, leveraging its native Protobuf and declarative output support. This change replaces the manual JSON/YAML encoding and jq filtering with a much cleaner and more robust implementation. Key changes: - Replaced the previous result processing logic with a pipeline using `go-jq-yamlformat`. - Utilized the `WithProtojsonInput()` option to correctly handle `spannerpb.ResultSet` messages without manual conversion. - Used the declarative `WithCompactJSONOutput()` and `WithRawJSONOutput()` options to handle formatting flags. - Updated and corrected unit tests to match the exact output of `jq` and cover all formatting options.
![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 @apstndb, 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 significantly overhauls the application's output generation. By integrating the go-jq-yamlformat library, I've replaced the custom and somewhat complex logic for JSON/YAML serialization and jq processing with a cleaner, more maintainable, and feature-rich solution. This streamlines the codebase and improves how results are presented.
Highlights
- Output Processing Refactor: I've refactored the application's output processing logic to leverage the
go-jq-yamlformatlibrary. This change replaces the previous manual JSON/YAML encoding andjqfiltering with a more robust and declarative implementation. - Dependency Updates: I've introduced
github.com/apstndb/go-jq-yamlformatas a new direct dependency and removedgithub.com/itchyny/gojqandgopkg.in/yaml.v3from the direct dependencies, simplifying the project's dependency graph for output handling. Other minor dependency versions were also updated. - Protobuf and Declarative Output: The new
go-jq-yamlformatlibrary natively handles Protobuf messages, eliminating the need for manualprotojsonconversions. It also supports declarative options for handling formatting flags (e.g., compact/pretty JSON, raw output). - Enhanced Test Coverage: I've added a new test file (
main_test.go) with comprehensive unit tests for theprocessResultsfunction, ensuring that all new formatting options (JSON, YAML,jqfiltering, raw output) behave as expected.
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 is currently in preview and 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 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
The pull request successfully refactors the output processing logic by integrating the go-jq-yamlformat library, which significantly simplifies the codebase by replacing manual JSON/YAML encoding and jq filtering. This change improves code clarity and robustness. The new unit tests for processResults are a welcome addition, ensuring the new logic behaves as expected across various formatting options. My feedback focuses on refining dependency management and further improving code modularity.
This commit addresses the feedback from the pull request review. - The CSV output logic has been extracted from `processResults` into a dedicated `writeCsvResults` function to improve modularity. - Ran `go mod tidy` to clean up the `go.mod` file and ensure that `spanemuboost` is correctly listed as an indirect dependency.
apstndb
left a comment
apstndb
left a comment
There was a problem hiding this comment.
Pull Request Review
Comments
go.mod
line 12
Thank you for the catch! I've addressed this in commit 1db7e24 by running go mod tidy.
main.go
line 332
Great suggestion! I've refactored the CSV logic into a separate writeCsvResults function in commit 1db7e24. Thanks for helping to improve the code's modularity.
|
Thank you for the review! I've addressed the comments in commit 1db7e24.
|
The lint job was failing because it did not download the necessary dependencies before running golangci-lint. This commit adds a `go mod download` step to ensure all modules are available, which should resolve the `typecheck` errors.
This commit updates the linting workflow to follow the current best practices for `actions/setup-go`. - Replaced the static Go version with `go-version-file: go.mod` to automatically use the version defined in the module file. - Removed the explicit `go mod download` step, as the caching mechanism in `actions/setup-go` handles this automatically based on the `go.sum` hash. - Removed the unnecessary `skip-cache` option from the linting action.
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request refactors the output processing logic to use the go-jq-yamlformat library, replacing manual JSON/YAML encoding and jq filtering. The changes include removing the OpenCensus bridge, simplifying the output formatting logic, and updating unit tests. The code appears to be well-structured and addresses the stated goals of the refactoring.
| "github.com/apstndb/spannerotel/interceptor" | ||
| grpczap "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap" | ||
| "github.com/jessevdk/go-flags" | ||
| "go.opentelemetry.io/otel" |
There was a problem hiding this comment.
The oteloc import and oteloc.InstallTraceBridge() call have been removed. Ensure that the removal of the OpenCensus bridge does not negatively impact any existing integrations or dependencies that rely on it. If the bridge is no longer needed, this change is acceptable; otherwise, a migration plan or alternative solution should be considered.
| switch o.Format { | ||
| case "yaml": | ||
| executeOptions = append(executeOptions, jqyaml.WithWriter(w, jqyaml.FormatYAML)) | ||
| case "json": | ||
| executeOptions = append(executeOptions, jqyaml.WithWriter(w, jqyaml.FormatJSON)) | ||
| if o.JqRawOutput { | ||
| executeOptions = append(executeOptions, jqyaml.WithRawJSONOutput()) | ||
| } else { | ||
| if o.CompactOutput { | ||
| executeOptions = append(executeOptions, jqyaml.WithCompactJSONOutput()) | ||
| } else { | ||
| executeOptions = append(executeOptions, jqyaml.WithPrettyJSONOutput()) | ||
| } | ||
| } | ||
| default: | ||
| return fmt.Errorf("unknown format: %s", o.Format) | ||
| } |
There was a problem hiding this comment.
The switch statement for handling different output formats could benefit from a default case that returns an error, ensuring that any unhandled formats are explicitly flagged. This can prevent unexpected behavior if a new format is added but not properly handled in the switch.
default:
return fmt.Errorf("unknown format: %s", o.Format)| if err := csvWriter.WriteAll(records); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| err = csvWriter.WriteAll(records) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| csvWriter.Flush() | ||
| return csvWriter.Error() |
There was a problem hiding this comment.
The WriteAll and Flush operations on the csvWriter are separate. It's important to check for errors after Flush to ensure all data is written successfully. The current implementation only checks the error from WriteAll, potentially missing errors during the flush operation.
if err := csvWriter.WriteAll(records); err != nil {
return err
}
csvWriter.Flush()
return csvWriter.Error()
Refactor the output processing logic to use the go-jq-yamlformat library, leveraging its native Protobuf and declarative output support.
This change replaces the manual JSON/YAML encoding and jq filtering with a much cleaner and more robust implementation.
Key changes:
go-jq-yamlformat.WithProtojsonInput()option to correctly handlespannerpb.ResultSetmessages without manual conversion.WithCompactJSONOutput()andWithRawJSONOutput()options to handle formatting flags.jqand cover all formatting options.