fix: register and consume --json shorthand for custom-format shortcuts#1737
Open
zhaojunlin0405 wants to merge 9 commits into
Open
fix: register and consume --json shorthand for custom-format shortcuts#1737zhaojunlin0405 wants to merge 9 commits into
zhaojunlin0405 wants to merge 9 commits into
Conversation
zhaojunlin0405
added
bug
|
|
github-actions
Bot
added
the
size/L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds shared ChangesJSON shorthand and format validation
Estimated code review effort: 3 (Moderate) | ~25 minutes 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@4add473509c1a087361d82feb07c608d2439eb9b🧩 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.42% -0.09%
==========================================
Files 852 854 +2
Lines 87655 88485 +830
==========================================
+ Hits 65312 65859 +547
- Misses 17315 17555 +240
- Partials 5028 5071 +43 ☔ 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-triage.md,lark-mail-watch.md,skills/lark-base/references/lark-base-data-query.md: document the shorthand, the explicit-format precedence, and JSON-output guidance for machine-consumption scenariosshortcuts/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
--jsonshorthand support for relevant--format-based commands (e.g., record list/search/get and mail commands), documented as an alias for--format json.--formatoptions for+record-list,mail +triage, andmail +watchwith explicit allowed values.--jsoncorrectly sets output to JSON when appropriate, while preserving commands where--jsonrepresents request-body data.--formatoverrides--json, and validation reports allowed values clearly.--jsonvs--formatbehavior andrequest-bodyvs output formatting distinctions.