Copilot commented on code in PR #118:
URL: https://github.com/apache/cloudstack-go/pull/118#discussion_r2313151755
##########
generate/generate.go:
##########
@@ -1363,7 +1376,10 @@ func (s *service) generateConvertCode(cmd, name, typ
string) {
case "map[string]string":
pn("m := v.(map[string]string)")
zeroIndex := detailsRequireZeroIndex[cmd]
- if zeroIndex {
+ needsIndex := parametersRequireIndexing[name]
+
+ // Use index variable if parameter requires it or command
doesn't use zero indexing
Review Comment:
[nitpick] The condition logic could be clearer. Consider renaming the
boolean or adding a comment to explain that this determines when to omit the
index variable in the loop. The current logic reads as 'if zero index AND not
needs index' which is confusing without context.
##########
generate/generate.go:
##########
@@ -1408,7 +1424,7 @@ func (s *service) generateConvertCode(cmd, name, typ
string) {
pn(" u.Set(fmt.Sprintf(\"%s[%%d].name\", i), k)",
name)
pn(" u.Set(fmt.Sprintf(\"%s[%%d].value\", i),
m[k])", name)
default:
- if zeroIndex && !detailsRequireKeyValue[cmd] {
+ if zeroIndex && !detailsRequireKeyValue[cmd] &&
!needsIndex {
Review Comment:
[nitpick] This complex condition with three boolean checks makes the logic
hard to follow. Consider extracting this into a well-named boolean variable
like `shouldUseStaticZeroIndex` to improve readability and make the intent
clearer.
```suggestion
shouldUseStaticZeroIndex := zeroIndex &&
!detailsRequireKeyValue[cmd] && !needsIndex
if shouldUseStaticZeroIndex {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
