I read through all five changed files and traced the full execution path from the struct tag down through fromstruct.go, mflags.go, and values.go. The implementation is clean and correct.
What's right:
noSplit flag is a one-line boolean added to stringArrayValue. The existing Set path is split cleanly — when noSplit is true, the value is appended as-is; otherwise the existing comma-split path runs unchanged. No existing behaviour is touched.StringArrayNoSplitVar mirrors StringArrayVar exactly in its nil-vs-empty-default semantics, so the two behave consistently.fromstruct.go handles the three valid tag values ("", "true", "false") and returns a descriptive error for anything else — that's the right design; silent ignoring would be worse.Type() returning "value" (instead of "value,...") is a useful help-text improvement; it accurately signals that the flag doesn't accept comma-separated lists.stringarray_nosplit_test.go) covers the meaningful cases: single comma-containing value, multiple occurrences with mixed syntax (-e, -eVAL, --env=VAL), default-replacement-on-first-set, nil default, split:"false" and split:"true" through FromStruct, and the invalid tag value rejection.One small gap worth noting:
There is a non-Var convenience form StringArray(…) *[]string for the splitting variant, but no StringArrayNoSplit(…) *[]string equivalent. This isn't a blocker — FromStruct and StringArrayNoSplitVar are both sufficient — but callers who build flag sets imperatively rather than via structs would have to use the Var form. Worth adding in a follow-up if the API surface is important for consistency.
Everything else looks solid. No correctness bugs, no regressions to existing behaviour, tests are meaningful and passing. Ready to merge.
Verdict: ready
{
"owner": "mirendev",
"repo": "mflags",
"number": 16,
"verdict": "ready",
"event": "approve",
"summary": "I read through all five changed files and traced the full execution path from the struct tag down through `fromstruct.go`, `mflags.go`, and `values.go`. The implementation is clean and correct.\n\n**What's right:**\n\n- The `noSplit` flag is a one-line boolean added to `stringArrayValue`. The existing `Set` path is split cleanly — when `noSplit` is true, the value is appended as-is; otherwise the existing comma-split path runs unchanged. No existing behaviour is touched.\n- `StringArrayNoSplitVar` mirrors `StringArrayVar` exactly in its nil-vs-empty-default semantics, so the two behave consistently.\n- `fromstruct.go` handles the three valid tag values (`\"\"`, `\"true\"`, `\"false\"`) and returns a descriptive error for anything else — that's the right design; silent ignoring would be worse.\n- `Type()` returning `\"value\"` (instead of `\"value,...\"`) is a useful help-text improvement; it accurately signals that the flag doesn't accept comma-separated lists.\n- The test file (`stringarray_nosplit_test.go`) covers the meaningful cases: single comma-containing value, multiple occurrences with mixed syntax (`-e`, `-eVAL`, `--env=VAL`), default-replacement-on-first-set, nil default, `split:\"false\"` and `split:\"true\"` through `FromStruct`, and the invalid tag value rejection.\n\n**One small gap worth noting:**\n\nThere is a non-Var convenience form `StringArray(…) *[]string` for the splitting variant, but no `StringArrayNoSplit(…) *[]string` equivalent. This isn't a blocker — `FromStruct` and `StringArrayNoSplitVar` are both sufficient — but callers who build flag sets imperatively rather than via structs would have to use the `Var` form. Worth adding in a follow-up if the API surface is important for consistency.\n\nEverything else looks solid. No correctness bugs, no regressions to existing behaviour, tests are meaningful and passing. Ready to merge.",
"posted_to_pr": true
}