fix(cli): stop root --help listing per-command flags as global - #1223
Conversation
📝 WalkthroughWalkthroughReplace the long embedded FLAGS: section in the rootLong help text with a short note that most flags are per-command and point users to "lark-cli --help"; add a unit test to assert that phrasing is present. ChangesHelp Text and Test
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@5428ba7843cbb63cd5d31b17bddc4fc46ef696ee🧩 Skill updatenpx skills add larksuite/cli#fix/root-help-flags-drift -y -g |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1223 +/- ##
=======================================
Coverage 69.19% 69.19%
=======================================
Files 634 634
Lines 59482 59482
=======================================
Hits 41161 41161
Misses 15007 15007
Partials 3314 3314 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d991ee4 to
50b10c3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/root_test.go (1)
86-98: ⚡ Quick winAlso assert the legacy static
FLAGS:block is gone.This only checks the new pointer text. If someone reintroduces the old hand-written
FLAGS:section alongside it, the test still passes and the original drift comes back. A negative assertion on the legacyFLAGS:header would pin the actual regression this PR is fixing.Suggested test tightening
func TestRootLong_FlagsSectionIsPerCommandPointer(t *testing.T) { // The flags shown in root help live on leaf commands (api, service), not // on the root command — every one errors "unknown flag" at the top level. // So the FLAGS section must point to `<command> --help` and explicitly say // the flags are per-command, rather than re-list them as if they were // global (which both lies and silently drifts as flags/defaults change). if !strings.Contains(rootLong, "per-command") { t.Fatalf("root help FLAGS section must say flags are per-command, got:\n%s", rootLong) } if !strings.Contains(rootLong, "lark-cli <command> --help") { t.Fatalf("root help FLAGS section must point to `<command> --help` for the authoritative flag list, got:\n%s", rootLong) } + if strings.Contains(rootLong, "\nFLAGS:\n") { + t.Fatalf("root help must not carry a static FLAGS block anymore, got:\n%s", rootLong) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/root_test.go` around lines 86 - 98, The test TestRootLong_FlagsSectionIsPerCommandPointer only asserts the presence of the new pointer text but not the absence of the legacy static header; update the test to also assert that rootLong does NOT contain the legacy "FLAGS:" block by adding a negative check (using strings.Contains on rootLong and failing with t.Fatalf) to ensure the old hard-coded "FLAGS:" header is gone alongside the new pointer checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/root_test.go`:
- Around line 86-98: The test TestRootLong_FlagsSectionIsPerCommandPointer only
asserts the presence of the new pointer text but not the absence of the legacy
static header; update the test to also assert that rootLong does NOT contain the
legacy "FLAGS:" block by adding a negative check (using strings.Contains on
rootLong and failing with t.Fatalf) to ensure the old hard-coded "FLAGS:" header
is gone alongside the new pointer checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f82afb6e-5d1f-4f08-948b-454097a868fc
📒 Files selected for processing (2)
cmd/root.gocmd/root_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/root.go
50b10c3 to
b637ce1
Compare
The hand-written FLAGS block in `lark-cli --help` listed --params, --data, --as, --format, --page-all, --page-size, --page-limit, --page-delay, -o, --jq, -q and --dry-run as if they were global flags. None are registered on the root command — they all error "unknown flag" at the top level and exist only on leaf commands (api, service). The block also contradicted the Cobra-generated "Flags:" section rendered directly below it, which shows only -h/--help, --profile, -v/--version. Replace it with a short illustrative example list (common flags first) and a pointer to `lark-cli <command> --help` for the full per-command set. Root help stays a discovery signpost without claiming the flags are global or restating defaults/descriptions that drift from the real flag sets. Change-Id: Ia1cab889dd70b6b49a61dac468dedfd7fe39043f
b637ce1 to
5428ba7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/root_test.go (1)
86-95: ⚡ Quick winAlso assert the stale root-only flag table is gone.
This only checks that the new pointer text exists. It would still pass if
rootLongkept the old misleadingFLAGS:cheat-sheet and merely appended the pointer, which is the regression this PR is trying to prevent. Add at least one negative assertion for a representative old entry (for example--page-allor--dry-run) or for the old exhaustive block header so the test pins the removal as well as the replacement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/root_test.go` around lines 86 - 95, Update the TestRootLong_FlagsSectionPointsToCommandHelp test to also assert that the stale root-only flag table is gone: in addition to checking that rootLong contains the pointer "lark-cli <command> --help", add a negative assertion that rootLong does NOT contain a representative old entry or header (e.g., the string "--page-all" or the old "FLAGS:" exhaustive block header or "--dry-run") so the test fails if the old cheat-sheet remains; locate the test function TestRootLong_FlagsSectionPointsToCommandHelp and add the negative contains check against the rootLong variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/root_test.go`:
- Around line 86-95: Update the TestRootLong_FlagsSectionPointsToCommandHelp
test to also assert that the stale root-only flag table is gone: in addition to
checking that rootLong contains the pointer "lark-cli <command> --help", add a
negative assertion that rootLong does NOT contain a representative old entry or
header (e.g., the string "--page-all" or the old "FLAGS:" exhaustive block
header or "--dry-run") so the test fails if the old cheat-sheet remains; locate
the test function TestRootLong_FlagsSectionPointsToCommandHelp and add the
negative contains check against the rootLong variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87a60535-ac03-4712-aa71-f859ca284756
📒 Files selected for processing (2)
cmd/root.gocmd/root_test.go
✅ Files skipped from review due to trivial changes (1)
- cmd/root.go
Follow-up to #1223. The hand-written FLAGS block in `lark-cli --help` restated leaf-command flags at the root level — flags that are not registered on the root command (they error "unknown flag" there). Even trimmed to an illustrative example list, it duplicated information Cobra's per-command `--help` already renders authoritatively, and any static list in root help drifts from the real per-command flag sets over time. Drop the section entirely: Cobra's per-command `Flags:` output is the single source of truth. `USAGE:`/`EXAMPLES:` still show flags in context, and the `Flags:` block at the bottom of root help lists the actual root flags. Also removes the now-obsolete TestRootLong_FlagsSectionPointsToCommandHelp.
…uite#1223) The hand-written FLAGS block in `lark-cli --help` listed --params, --data, --as, --format, --page-all, --page-size, --page-limit, --page-delay, -o, --jq, -q and --dry-run as if they were global flags. None are registered on the root command — they all error "unknown flag" at the top level and exist only on leaf commands (api, service). The block also contradicted the Cobra-generated "Flags:" section rendered directly below it, which shows only -h/--help, --profile, -v/--version. Replace it with a short illustrative example list (common flags first) and a pointer to `lark-cli <command> --help` for the full per-command set. Root help stays a discovery signpost without claiming the flags are global or restating defaults/descriptions that drift from the real flag sets. Change-Id: Ia1cab889dd70b6b49a61dac468dedfd7fe39043f
Follow-up to larksuite#1223. The hand-written FLAGS block in `lark-cli --help` restated leaf-command flags at the root level — flags that are not registered on the root command (they error "unknown flag" there). Even trimmed to an illustrative example list, it duplicated information Cobra's per-command `--help` already renders authoritatively, and any static list in root help drifts from the real per-command flag sets over time. Drop the section entirely: Cobra's per-command `Flags:` output is the single source of truth. `USAGE:`/`EXAMPLES:` still show flags in context, and the `Flags:` block at the bottom of root help lists the actual root flags. Also removes the now-obsolete TestRootLong_FlagsSectionPointsToCommandHelp.
Problem
lark-cli --helpprinted two flag lists that contradict each other. The hand-writtenFLAGS:block listed a dozen flags (--params,--data,--format,--page-all,--jq,-o, …) as if they were global — but none are registered on the root command. They all error at the top level and exist only on the leaf commands (api,service):Cobra's own
Flags:section, rendered right below, lists the real root flags — only-h/--help,--profile,-v/--version. The static block also flattened away the fact that the flag set varies by command (e.g.contact +search-userhas--page-sizebut no--page-all/--page-limit/--page-delay) and hand-restated defaults that have to be kept in sync manually.Fix
Before (excerpt — 12 entries, each with description + defaults):
After:
A short illustrative example list — the cross-cutting flags that work on any command, with
...signalling it is not exhaustive — plus a pointer to the authoritative per-command source. Root help stays a discovery signpost without claiming the flags are global or restating defaults/descriptions that drift from the real flag sets.USAGE:/EXAMPLES:still show the flags in their correct position, after a<command>.Tests
go test ./cmd/ -run TestRootLongpasses.TestRootLong_FlagsSectionPointsToCommandHelpguards against reverting to a standalone exhaustive root flag table (asserts the section points tolark-cli <command> --help).Summary by CodeRabbit