fix: register and consume --json shorthand for custom-format shortcuts#1737
Conversation
zhaojunlin0405
added
bug
|
|
github-actions
Bot
added
the
size/L
📝 WalkthroughWalkthroughThis PR centralizes Changes--json shorthand and format enum updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CobraCommand
participant runner.go
participant RuntimeContext
User->>CobraCommand: run shortcut with --json
CobraCommand->>runner.go: registerShortcutFlagsWithContext
runner.go->>runner.go: ensureJSONShorthand(cmd, s)
runner.go-->>CobraCommand: --json registered if safe
CobraCommand->>runner.go: newRuntimeContext(cmd, s)
runner.go->>runner.go: applyJSONShorthand(cmd, s)
alt --json set and no explicit --format
runner.go->>runner.go: set format=json
else explicit --format provided
runner.go->>runner.go: keep user-specified format
end
runner.go->>RuntimeContext: cache Format value
RuntimeContext-->>User: command executes with resolved format
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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@de384c02667e3d164751570ee6bf7562491c4ead🧩 Skill updatenpx skills add larksuite/cli#fix/mail-triage-json-shorthand -y -g |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1737 +/- ##
==========================================
- Coverage 74.51% 74.40% -0.11%
==========================================
Files 852 860 +8
Lines 87655 89460 +1805
==========================================
+ Hits 65312 66564 +1252
- Misses 17315 17744 +429
- Partials 5028 5152 +124 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
shortcuts/common/runner_json_shorthand_test.go (1)
29-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment language consistency.
Test comments are in Chinese while the rest of the codebase (e.g. runner.go) documents in English, which may hinder readability for the broader contributor base.
Also applies to: 49-49, 61-61, 73-73, 110-110, 120-120, 130-130, 140-140, 150-150, 170-170, 187-187
🤖 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 `@shortcuts/common/runner_json_shorthand_test.go` around lines 29 - 30, The test comments in runner_json_shorthand_test.go are written in Chinese while the surrounding codebase documentation is in English, so update the affected comment blocks to English for consistency and readability. Keep the existing test names and behavior unchanged, and revise the inline comments near the JSON shorthand tests so they clearly describe the cases in English across all referenced occurrences.shortcuts/mail/mail_watch.go (1)
102-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueManual format validation now redundant after adding
Enum.With
Enum: []string{"json", "data"}on theformatflag (Line 102), the framework will reject any other value beforeExecuteruns — confirmed byTestMailTriageEnumRejectsUnknownFormat's dry-run test rejecting invalid values without reaching business logic. Theswitch/defaulterror branch at Lines 188-192 is therefore dead code for real invocations.Consider removing it (or documenting why it's kept as defense-in-depth) to avoid confusing future maintainers about where validation actually happens.
♻️ Proposed cleanup
outFormat := runtime.Str("format") - switch outFormat { - case "json", "data", "": - default: - return mailValidationParamError("--format", "invalid --format %q: must be json or data", outFormat) - } msgFormat := runtime.Str("msg-format")Also applies to: 188-192
🤖 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 `@shortcuts/mail/mail_watch.go` at line 102, The format validation in MailWatch is now duplicated because the format flag already has an Enum on the flags definition, so the framework rejects invalid values before execution reaches business logic. Remove the unreachable default/error branch in MailWatch.Execute that handles unknown format values, or keep it only if you add an explicit comment explaining it as defense-in-depth; use the format flag definition and the Execute switch on format as the symbols to locate the cleanup.
🤖 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.
Inline comments:
In `@shortcuts/mail/mail_json_shorthand_test.go`:
- Around line 80-92: The test in TestMailTriageEnumRejectsUnknownFormat
currently verifies only error message substrings; update it to assert typed
error metadata instead. Use errs.ProblemOf on the error returned by
runMountedMailShortcut to check the expected category/subtype/param for the
invalid --format value, and also verify cause preservation rather than relying
on strings.Contains checks on err.Error().
---
Nitpick comments:
In `@shortcuts/common/runner_json_shorthand_test.go`:
- Around line 29-30: The test comments in runner_json_shorthand_test.go are
written in Chinese while the surrounding codebase documentation is in English,
so update the affected comment blocks to English for consistency and
readability. Keep the existing test names and behavior unchanged, and revise the
inline comments near the JSON shorthand tests so they clearly describe the cases
in English across all referenced occurrences.
In `@shortcuts/mail/mail_watch.go`:
- Line 102: The format validation in MailWatch is now duplicated because the
format flag already has an Enum on the flags definition, so the framework
rejects invalid values before execution reaches business logic. Remove the
unreachable default/error branch in MailWatch.Execute that handles unknown
format values, or keep it only if you add an explicit comment explaining it as
defense-in-depth; use the format flag definition and the Execute switch on
format as the symbols to locate the cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49f57623-1d3b-40eb-b5ca-f7fdf145065e
📒 Files selected for processing (10)
shortcuts/base/record_json_shorthand_test.goshortcuts/base/record_list.goshortcuts/common/runner.goshortcuts/common/runner_json_shorthand_test.goshortcuts/mail/mail_json_shorthand_test.goshortcuts/mail/mail_triage.goshortcuts/mail/mail_watch.goskills/lark-base/references/lark-base-data-query.mdskills/lark-mail/references/lark-mail-triage.mdskills/lark-mail/references/lark-mail-watch.md
bubbmon233
left a comment
bubbmon233
left a comment
There was a problem hiding this comment.
只看 mail 这块，有一个文档不一致需要修一下：mail +watch 的 skill 文档里还写着 --format 默认是 table，支持 table / json / data，但代码现在实际是默认 data，并且 Enum 只允许 json / data。这次 PR 加了 enum 校验后，用户照文档传 --format table 会直接失败。
建议把 skills/lark-mail/references/lark-mail-watch.md 里的参数说明改成类似：--format <mode> 默认 data，输出样式：json / data。
|
@bubbmon233 Good catch — fixed in 16ba718. The |
4 participants
Summary
--json(shorthand for--format json) was only registered when the framework injected the default format flag, so shortcuts declaring their own format flag —mail +triage,mail +watch,base +record-list— rejected it withunknown flag: --json. In addition, nothing ever consumed the flag's value: it appeared to work on other commands only because their format already defaults to json, and--format table --jsonsilently ignored the shorthand.This change decouples shorthand registration from default-format injection and adds an explicit consumption step:
ensureJSONShorthandregisters--jsonwhen a command has a format flag whose Enum containsjsonand thejsonflag name is not already taken.applyJSONShorthandfolds the shorthand into the format flag itself before it is consumed, so both the cached format and per-command reads observejson. An explicitly passed--formatalways wins over the shorthand.--json(event +subscribe's pretty-print switch,base +record-search/+record-get's request-body payload) are untouched on both the registration and consumption paths.Behavior change: invalid
--formatvalues on the five affected commands now fail enum validation (exit 2, listing allowed values) instead of silently falling back to the default format (mail +triage) or erroring later inside Execute (the others).Changes
shortcuts/common/runner.go: addshortcutDeclaresJSONFlag/shortcutFormatSupportsJSON/ensureJSONShorthand/applyJSONShorthand; decouple the--jsonregistration from the default-format injection block; apply normalization innewRuntimeContextbefore the format value is cachedshortcuts/mail/mail_triage.go,shortcuts/mail/mail_watch.go,shortcuts/base/record_list.go: add Enum metadata to the existing format flag declarations (one line each; defaults unchanged)skills/lark-mail/references/lark-mail-watch.md: correct the--formatrow (defaultdata, valuesjson/data) so the docs match the added Enum. The--jsonshorthand is deliberately left undocumented — it stays a silent fallback (typing--jsonno longer errors), while the skill docs keep steering agents to--format jsonshortcuts/common/runner_json_shorthand_test.go(registration matrix + normalization precedence),shortcuts/mail/mail_json_shorthand_test.go,shortcuts/base/record_json_shorthand_test.goTest Plan
go test ./shortcuts/...: registration matrix (5 cases), normalization precedence (6 cases), mail behavior (help / dry-run equivalence / explicit-table precedence / enum error, 6 cases), base registration + request-body regression (3 cases) — all greengo build ./...and incremental golangci-lint clean--jsonand--format json, explicit--format table --jsonprecedence via the jq mutual-exclusion probe, enum validation errors on all five commands, andrecord-search/record-getrequest-body--jsonregression anchorsRelated Issues
unknown flag: --jsononmail +triage; the same registration gap onmail +watchandbase +record-listis fixed by the shared rulemail +triage --jqis accepted but silently ignored because the command's custom output path bypasses the common jq filterSummary by CodeRabbit
New Features
--jsonshorthand for several record and mail commands, with help text showing when it’s available.Bug Fixes
--jsonfrom overriding commands that already use--jsonfor other input types.--formattakes precedence when both format options are provided.Documentation