feat: Add structured output for net-info #3029
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
Summary of ChangesHello @alpe, 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 enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
|
Claude finished @alpe's task in 1m 52s —— View job Code Review CompleteI've completed a comprehensive review of PR #3029. The implementation successfully adds JSON output support and improves RPC address handling. Below are my findings organized by priority. Critical Issues1. Missing Output Format Validation (pkg/cmd/p2p.go:83-85)Priority: High | Category: Error Handling The current implementation silently falls back to text output for any unsupported format value. This creates a poor user experience and makes typos hard to detect. if outputFormat == "json" {
return formatJson(cmd.OutOrStdout(), netInfo, peerResp)
}
// Falls through to text output for ANY other valueRecommendation: switch outputFormat {
case "json":
return formatJson(cmd.OutOrStdout(), netInfo, peerResp)
case "text":
// proceed with text output
default:
return fmt.Errorf("unsupported output format: %q, expected 'text' or 'json'", outputFormat)
}Major Issues2. Missing Test Coverage for New JSON Feature (30% patch coverage)Priority: High | Category: Testing The PR adds 40 lines of new code (JSON formatting logic and flag handling), but the existing tests in Missing test cases:
Recommendation: Add tests following the existing pattern: func TestNetInfoCmd_JSONOutput(t *testing.T) {
// Test JSON output with peers
}
func TestNetInfoCmd_JSONOutput_NoPeers(t *testing.T) {
// Test JSON output without peers
}
func TestNetInfoCmd_InvalidOutputFormat(t *testing.T) {
// Test error handling for invalid format
}
func TestNetInfoCmd_HTTPSAddress(t *testing.T) {
// Test RPC address with https:// prefix
}Add tests →%20JSON%20output%20with%20peers%2C%202)%20JSON%20output%20without%20peers%2C%203)%20invalid%20output%20format%20validation%2C%204)%20RPC%20address%20handling%20with%20http%3A%2F%2F%20and%20https%3A%2F%2F%20prefixes.%20Follow%20the%20existing%20test%20patterns%20in%20the%20file.&repo=evstack%2Fev-node&branch=alex%2Fremote_netinfo) 3. Function Coupling to Connect Library (pkg/cmd/p2p.go:134)Priority: Medium | Category: Architecture The Current signature: func formatJson(w io.Writer, netInfo *pb.NetInfo, peerResp *connect.Response[pb.GetPeerInfoResponse]) errorRecommended signature: func formatJson(w io.Writer, netInfo *pb.NetInfo, peers []*pb.PeerInfo) errorBenefits:
Call site update: if outputFormat == "json" {
return formatJson(cmd.OutOrStdout(), netInfo, peerResp.Msg.Peers)
}Fix this →%20Change%20formatJson%20function%20signature%20from%20%60connect.Response%5Bpb.GetPeerInfoResponse%5D%60%20to%20%60%5B%5Dpb.PeerInfo%60%20at%20line%20134%2C%202)%20Update%20the%20call%20site%20at%20line%2084%20to%20pass%20%60peerResp.Msg.Peers%60%20directly%2C%203)%20Update%20function%20body%20at%20line%20145%20to%20use%20the%20peers%20parameter%20directly&repo=evstack%2Fev-node&branch=alex%2Fremote_netinfo) Minor Issues4. Inconsistent Error Message Format (pkg/cmd/p2p.go:65, 75)Priority: Low | Category: Code Quality Error messages changed from verbose Before: return fmt.Errorf("error calling GetNetInfo RPC: %w", err)After: return fmt.Errorf("GetNetInfo RPC: %w", err)Recommendation: Consider consistency with other commands in the codebase. If brevity is preferred project-wide, this is acceptable. Otherwise, consider: return fmt.Errorf("failed to call GetNetInfo RPC: %w", err)5. Context Usage Improvement (pkg/cmd/p2p.go:61, 71)Priority: Low | Category: Code Quality Good improvement replacing ✅ Well done - This change properly respects command lifecycle and user interrupts. 6. Potential Nil Dereference (pkg/cmd/p2p.go:145-146)Priority: Low | Category: Defensive Programming While the RPC layer likely ensures func formatJson(w io.Writer, netInfo *pb.NetInfo, peerResp *connect.Response[pb.GetPeerInfoResponse]) error {
if peerResp == nil || peerResp.Msg == nil {
return fmt.Errorf("invalid peer response")
}
// ... rest of function
}However, if the RPC contract guarantees non-nil responses on success, this may be unnecessary. Check the Connect library's guarantees. Positive Observations
Architecture & DesignThe overall design is clean and follows Go idioms well:
Security ConsiderationsNo significant security issues identified:
Performance ConsiderationsNo performance concerns:
SummaryThis is a solid feature addition that improves the tool's automation capabilities. The implementation is clean and follows Go best practices. The main areas for improvement are:
The 30% patch coverage is the most significant concern - the new JSON functionality should have dedicated test cases to ensure reliability. Todo List
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully introduces structured JSON output for the net-info command, which is a valuable enhancement. The implementation is clean, adding a new -o flag and separating the JSON formatting logic. The refactoring to handle RPC addresses more flexibly and to fetch data before formatting is also well done. I have a couple of suggestions to improve the robustness of the output format handling and to refine the new JSON formatting function for better decoupling and testability.
| if outputFormat == "json" { | ||
| return formatJson(cmd.OutOrStdout(), netInfo, peerResp) | ||
| } |
There was a problem hiding this comment.
The current implementation falls back to text output for any unsupported output format. It would be more user-friendly to validate the output flag and return an error for unsupported values. A switch statement can make this logic cleaner and more robust.
Additionally, to improve the formatJson function's signature and reduce its coupling to connect types, you can pass peerResp.Msg.Peers directly instead of the whole peerResp object.
| if outputFormat == "json" { | |
| return formatJson(cmd.OutOrStdout(), netInfo, peerResp) | |
| } | |
| switch outputFormat { | |
| case "json": | |
| return formatJson(cmd.OutOrStdout(), netInfo, peerResp.Msg.Peers) | |
| case "text": | |
| // valid format, proceed to text output | |
| default: | |
| return fmt.Errorf("unsupported output format: %q, please use 'text' or 'json'", outputFormat) | |
| } |
| func formatJson(w io.Writer, netInfo *pb.NetInfo, peerResp *connect.Response[pb.GetPeerInfoResponse]) error { | ||
| type peerJSON struct { | ||
| ID string `json:"id"` | ||
| Address string `json:"address"` | ||
| } | ||
| type netInfoJSON struct { | ||
| NodeID string `json:"node_id"` | ||
| ListenAddresses []string `json:"listen_addresses"` | ||
| Peers []peerJSON `json:"peers"` | ||
| } | ||
|
|
||
| peers := make([]peerJSON, 0, len(peerResp.Msg.Peers)) | ||
| for _, peer := range peerResp.Msg.Peers { | ||
| peers = append(peers, peerJSON{ | ||
| ID: peer.Id, | ||
| Address: peer.Address, | ||
| }) | ||
| } |
There was a problem hiding this comment.
To align with the suggested change at the call site, the function signature should be updated to accept []*pb.PeerInfo directly, rather than the *connect.Response wrapper. This makes the function's purpose clearer, decouples it from the connect library, and simplifies testing.
| func formatJson(w io.Writer, netInfo *pb.NetInfo, peerResp *connect.Response[pb.GetPeerInfoResponse]) error { | |
| type peerJSON struct { | |
| ID string `json:"id"` | |
| Address string `json:"address"` | |
| } | |
| type netInfoJSON struct { | |
| NodeID string `json:"node_id"` | |
| ListenAddresses []string `json:"listen_addresses"` | |
| Peers []peerJSON `json:"peers"` | |
| } | |
| peers := make([]peerJSON, 0, len(peerResp.Msg.Peers)) | |
| for _, peer := range peerResp.Msg.Peers { | |
| peers = append(peers, peerJSON{ | |
| ID: peer.Id, | |
| Address: peer.Address, | |
| }) | |
| } | |
| func formatJson(w io.Writer, netInfo *pb.NetInfo, peerInfos []*pb.PeerInfo) error { | |
| type peerJSON struct { | |
| ID string `json:"id"` | |
| Address string `json:"address"` | |
| } | |
| type netInfoJSON struct { | |
| NodeID string `json:"node_id"` | |
| ListenAddresses []string `json:"listen_addresses"` | |
| Peers []peerJSON `json:"peers"` | |
| } | |
| peers := make([]peerJSON, 0, len(peerInfos)) | |
| for _, peer := range peerInfos { | |
| peers = append(peers, peerJSON{ | |
| ID: peer.Id, | |
| Address: peer.Address, | |
| }) | |
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3029 +/- ##
==========================================
- Coverage 55.55% 55.45% -0.11%
==========================================
Files 116 116
Lines 11477 11507 +30
==========================================
+ Hits 6376 6381 +5
- Misses 4401 4424 +23
- Partials 700 702 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
alpe
changed the title
Resolves #3024
--evnode.rpc.addresswas existing already to point to a remote server-ofor structured json output. default is text as before