Adding error for network mismatch between CLI and config #1950
Conversation
Summary of ChangesHello @sergey3bv, 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 improves the user experience by introducing robust error handling for network mismatches between CLI commands and 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
|
![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 introduces a helpful feature to detect and report network mismatches between the CLI and the tapd node configuration. The implementation correctly detects the network from the config file and provides a user-friendly error with a suggested command.
I've found a critical bug in the command suggestion logic that needs to be addressed. Additionally, I've included a couple of medium-severity suggestions to improve the robustness of the config parsing and the overall code quality. The new tests are well-written and cover the new functionality effectively.
| for i, arg := range os.Args { | ||
| if arg == "--network" || arg == "-n" { | ||
| if i+1 < len(os.Args) { | ||
| args = append(args, arg, detectedNetwork) | ||
| foundNetworkFlag = true | ||
| continue | ||
| } | ||
| } | ||
| if strings.HasPrefix(arg, "--network=") { | ||
| args = append(args, "--network="+detectedNetwork) | ||
| foundNetworkFlag = true | ||
| continue | ||
| } | ||
| args = append(args, arg) | ||
| } |
There was a problem hiding this comment.
There's a bug in the logic to reconstruct the command arguments. When iterating over os.Args using a for...range loop, if you encounter a flag like --network or -n that takes a value as the next argument, you replace it but you don't skip the original value argument. The range loop will then process the old value in the next iteration, leading to an incorrect suggested command with both the old and new network values (e.g., ... -n new_network old_network ...).
To fix this, you should use a traditional for loop with an index, so you can manually increment the index to skip the next argument.
| for i, arg := range os.Args { | |
| if arg == "--network" || arg == "-n" { | |
| if i+1 < len(os.Args) { | |
| args = append(args, arg, detectedNetwork) | |
| foundNetworkFlag = true | |
| continue | |
| } | |
| } | |
| if strings.HasPrefix(arg, "--network=") { | |
| args = append(args, "--network="+detectedNetwork) | |
| foundNetworkFlag = true | |
| continue | |
| } | |
| args = append(args, arg) | |
| } | |
| for i := 0; i < len(os.Args); i++ { | |
| arg := os.Args[i] | |
| if (arg == "--network" || arg == "-n") && i+1 < len(os.Args) { | |
| args = append(args, arg, detectedNetwork) | |
| foundNetworkFlag = true | |
| i++ // Skip the old network value. | |
| continue | |
| } | |
| if strings.HasPrefix(arg, "--network=") { | |
| args = append(args, "--network="+detectedNetwork) | |
| foundNetworkFlag = true | |
| continue | |
| } | |
| args = append(args, arg) | |
| } |
| if after, ok :=strings.CutPrefix(line, "network="); ok { | ||
| network := after | ||
| network = strings.TrimSpace(network) | ||
| // Validate it's a known network | ||
| switch network { | ||
| case "mainnet", "testnet", "testnet4", "regtest", "simnet", "signet": | ||
| return network | ||
| } | ||
| } |
There was a problem hiding this comment.
The current parsing logic for the network setting is a bit brittle. It uses strings.CutPrefix(line, "network=") which will fail if there are any spaces around the = sign, for example network = mainnet.
To make this more robust and handle such common variations in config files, consider splitting the line by = and then trimming whitespace from the key and value.
| if after, ok :=strings.CutPrefix(line, "network="); ok { | |
| network := after | |
| network = strings.TrimSpace(network) | |
| // Validate it's a known network | |
| switch network { | |
| case "mainnet", "testnet", "testnet4", "regtest", "simnet", "signet": | |
| return network | |
| } | |
| } | |
| if parts := strings.SplitN(line, "=", 2); len(parts) == 2 && strings.TrimSpace(parts[0]) == "network" { | |
| network := strings.TrimSpace(parts[1]) | |
| // Validate it's a known network | |
| switch network { | |
| case "mainnet", "testnet", "testnet4", "regtest", "simnet", "signet": | |
| return network | |
| } | |
| } |
sergey3bv
force-pushed
the
feat/network-missmatch-error
branch
from
cbce47d to
c263b63
Compare
Roasbeef
requested review from
a team,
GeorgeTsagk and
jtobin
and removed request for
a team
1 participant
Addressing #1070