refactor(output)!: emitter follow-ups from PR #1899 (strict Format, pretty scan, jq safety, pagination merge)#1998
Open
sang-neo03 wants to merge 9 commits into
Open
refactor(output)!: emitter follow-ups from PR #1899 (strict Format, pretty scan, jq safety, pagination merge)#1998sang-neo03 wants to merge 9 commits into
sang-neo03 wants to merge 9 commits into
Conversation
…gal combos An unknown --format now fails with a typed ValidationError instead of printing a stderr warning and silently degrading to JSON, and unfulfillable combinations are rejected at the flag boundary rather than dropped silently. - Add FormatPretty to the Format enum and ParseFormatStrict, which returns a typed validation error (param --format) for any unrecognized format. - EmitOptions.Format / StreamOptions.Format / Emitter.streamFormat are now the typed output.Format; the upstream .String() -> ParseFormat round-trip and the Emitter's internal unknown-format fallback (printLegacyDataJSON) are removed. - Strict parsing runs at the api, service, and shortcut boundaries, before the dry-run branch so both dry-run and emit reject an unknown format. The strict contract applies only to the framework-injected --format; a shortcut that declares its own format flag (base +record-* markdown|json, mail +watch json|data) keeps its own enum, validated by validateEnumFlags. - The raw api/service commands reject --format pretty on the emit path (no response pretty renderer) while preserving the dry-run plain-text preview; the check runs before confirmation and client init. - ValidateJqFlags classifies the format via ParseFormat so --jq's JSON-only check is case-insensitive and single-sourced: --format JSON --jq no longer mis-rejects, while any non-JSON value (including a shortcut's markdown/data) still conflicts with --jq. BREAKING CHANGE: an unknown --format value (e.g. a typo like `--format tabel`) is now a typed validation error with a non-zero exit code instead of a stderr warning plus JSON output. Scripts that relied on the unknown-format JSON fallback must pass a valid format (json, ndjson, table, csv, or pretty).
The pretty path scanned the structured `data` argument but rendered via an opaque closure, so a renderer that printed content absent from `data` could bypass content-safety in block mode. Render pretty output into a buffer, run the safety scan on the actual rendered text, and only copy to stdout when it passes — the bytes that reach stdout are now exactly what was scanned. Applies to both Success's pretty path and StreamPage's pretty branch; json/table/csv/ ndjson are unchanged (they render from the scanned data directly).
…fety alert When --jq is applied, the jq expression can filter the _content_safety_alert field out of stdout, hiding the warning. Whether a stderr fallback warning was written used to depend on a caller-set EmitOptions.JQSafetyWarning flag, so the raw api/service paths warned but shortcut commands did not — the safety alert was silently lost. Remove the flag and always write the stderr warning when jq is applied and an alert exists.
…Output apiPaginate and servicePaginate were near-identical; merge them into one shared client.PaginateToOutput. The two call-site differences are injected: checkErr (both pass APIClient.CheckResponse) and markErr (cmd/api passes errs.MarkRaw, cmd/service passes nil). markErr wraps only the PaginateAll / StreamPages / checkErr errors — never the WriteSuccessEnvelope return — preserving both commands' exact stdout/stderr bytes and error semantics. Pure refactor.
github-actions
Bot
added
domain/calendar
|
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)
📝 WalkthroughWalkthroughThe CLI now strictly validates output formats, adds typed ChangesFormat contracts and emission
Centralized pagination
Shortcut integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant APIServiceCommand
participant PaginateToOutput
participant APIClient
participant Emitter
APIServiceCommand->>PaginateToOutput: provide format and pagination options
PaginateToOutput->>APIClient: request pages
APIClient-->>PaginateToOutput: return page data
PaginateToOutput->>Emitter: emit streamed or aggregated output
Emitter-->>APIServiceCommand: write output or return error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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@da5b75290715d28277453c0c23bbc53138cf59ee🧩 Skill updatenpx skills add larksuite/cli#refactor/output-emitter-followups -y -g |
![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 (1)
cmd/api/api.go (1)
261-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared pretty-rejection contract into one helper.
cmd/api/api.goandcmd/service/service.goeach hardcode the identicalFormatPrettyrejection (same subtype, message, and param) for their raw emit paths. A single shared helper keeps the two raw commands' "pretty is shortcut-only" contract from silently drifting apart.
cmd/api/api.go#L261-L268: replace the inline check with a call to a shared helper (e.g.output.RejectPretty(format)).cmd/service/service.go#L415-L423: replace the inline check with the same shared helper call.♻️ Proposed shared helper
// in internal/output (e.g. format_type.go) // RejectPretty returns a typed validation error if f is FormatPretty. Raw // api/service commands have no pretty renderer for responses and must reject // it before client init rather than silently falling back. func RejectPretty(f Format) error { if f != FormatPretty { return nil } return errs.NewValidationError(errs.SubtypeInvalidArgument, "--format pretty is not supported here (use json, ndjson, table, or csv)"). WithParam("--format") }- if format == output.FormatPretty { - return errs.NewValidationError(errs.SubtypeInvalidArgument, - "--format pretty is not supported here (use json, ndjson, table, or csv)"). - WithParam("--format") - } + if err := output.RejectPretty(format); err != nil { + return err + }🤖 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/api/api.go` around lines 261 - 268, Extract the identical FormatPretty validation into a shared output.RejectPretty helper that returns nil for other formats and preserves the existing typed error, message, and parameter. Replace the inline checks in cmd/api/api.go lines 261-268 and cmd/service/service.go lines 415-423 with calls to this helper.
🤖 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 `@cmd/api/api_test.go`:
- Around line 124-184: Add test isolation for the cmd/api tests by setting
LARKSUITE_CLI_CONFIG_DIR to a per-test temporary directory via t.Setenv near the
setup in TestApiCmd_UnknownFormat_Rejected, TestApiCmd_Pretty_RejectedOnEmit,
and TestApiCmd_Pretty_PreservedOnDryRun, or establish equivalent package-wide
isolation with TestMain. Ensure cmdutil.TestFactory cannot access the
developer’s real configuration.
---
Nitpick comments:
In `@cmd/api/api.go`:
- Around line 261-268: Extract the identical FormatPretty validation into a
shared output.RejectPretty helper that returns nil for other formats and
preserves the existing typed error, message, and parameter. Replace the inline
checks in cmd/api/api.go lines 261-268 and cmd/service/service.go lines 415-423
with calls to this helper.
🪄 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: e4f75c0e-b4fe-48d1-ba65-a4c77d9df2f6
📒 Files selected for processing (19)
cmd/api/api.gocmd/api/api_paginate_test.gocmd/api/api_test.gocmd/service/service.gocmd/service/service_paginate_test.gocmd/service/service_test.gointernal/client/paginate_emit.gointernal/client/response.gointernal/output/emitter.gointernal/output/emitter_contract_test.gointernal/output/emitter_legacy_compat_test.gointernal/output/envelope_success.gointernal/output/format_type.gointernal/output/format_type_test.gointernal/output/jq.gointernal/output/jq_test.gointernal/output/testdata/runtime_context_legacy.golden.jsonshortcuts/calendar/calendar_test.goshortcuts/common/runner.go
Comment on lines
+124
to
+184
| func TestApiCmd_UnknownFormat_Rejected(t *testing.T) { | ||
| for _, extra := range [][]string{nil, {"--dry-run"}} { | ||
| name := "emit" | ||
| if len(extra) > 0 { | ||
| name = "dry-run" | ||
| } | ||
| t.Run(name, func(t *testing.T) { | ||
| f, stdout, _, _ := cmdutil.TestFactory(t, &core.CliConfig{ | ||
| AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu, | ||
| }) | ||
| cmd := newTestApiCmd(f, nil) | ||
| cmd.SetArgs(append([]string{"GET", "/open-apis/test", "--as", "bot", "--format", "bogus"}, extra...)) | ||
| err := cmd.Execute() | ||
| if err == nil { | ||
| t.Fatal("expected a validation error for unknown --format") | ||
| } | ||
| requireProblem(t, err, errs.CategoryValidation, errs.SubtypeInvalidArgument, 0) | ||
| if !strings.Contains(err.Error(), "unknown output format") { | ||
| t.Errorf("error = %v, want unknown-format message", err) | ||
| } | ||
| if stdout.String() != "" { | ||
| t.Errorf("unknown --format must not write stdout, got:\n%s", stdout.String()) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // pretty is shortcut-only: the raw api command rejects it on the emit path | ||
| // (before client init) but keeps the dry-run plain-text preview. | ||
| func TestApiCmd_Pretty_RejectedOnEmit(t *testing.T) { | ||
| f, stdout, _, _ := cmdutil.TestFactory(t, &core.CliConfig{ | ||
| AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu, | ||
| }) | ||
| cmd := newTestApiCmd(f, nil) | ||
| cmd.SetArgs([]string{"GET", "/open-apis/test", "--as", "bot", "--format", "pretty"}) | ||
| err := cmd.Execute() | ||
| if err == nil { | ||
| t.Fatal("expected a validation error for --format pretty on the emit path") | ||
| } | ||
| requireProblem(t, err, errs.CategoryValidation, errs.SubtypeInvalidArgument, 0) | ||
| if !strings.Contains(err.Error(), "pretty") { | ||
| t.Errorf("error = %v, want pretty-not-supported message", err) | ||
| } | ||
| if stdout.String() != "" { | ||
| t.Errorf("rejected --format pretty must not write stdout, got:\n%s", stdout.String()) | ||
| } | ||
| } | ||
|
|
||
| func TestApiCmd_Pretty_PreservedOnDryRun(t *testing.T) { | ||
| f, stdout, _, _ := cmdutil.TestFactory(t, &core.CliConfig{ | ||
| AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu, | ||
| }) | ||
| cmd := newTestApiCmd(f, nil) | ||
| cmd.SetArgs([]string{"GET", "/open-apis/test", "--as", "bot", "--format", "pretty", "--dry-run"}) | ||
| if err := cmd.Execute(); err != nil { | ||
| t.Fatalf("dry-run --format pretty must be accepted, got: %v", err) | ||
| } | ||
| if !strings.Contains(stdout.String(), "# dry-run: request not sent") { | ||
| t.Fatalf("dry-run --format pretty lost its plain-text preview, stdout:\n%s", stdout.String()) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether cmdutil.TestFactory already isolates LARKSUITE_CLI_CONFIG_DIR via t.TempDir()/t.Setenv.
fd -t f 'testfactory|factory_test|testing' internal/cmdutil -a | xargs -r rg -nP -C4 'func TestFactory'
rg -nP -C4 'LARKSUITE_CLI_CONFIG_DIR|t\.TempDir\(\)|t\.Setenv' internal/cmdutilRepository: larksuite/cli
Length of output: 18769
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the cmd/api tests and the test factory/config resolution path.
ast-grep outline cmd/api/api_test.go --view expanded
printf '\n--- internal/cmdutil/testing.go ---\n'
cat -n internal/cmdutil/testing.go | sed -n '1,220p'
printf '\n--- internal/cmdutil/factory.go ---\n'
cat -n internal/cmdutil/factory.go | sed -n '1,260p'
printf '\n--- cmd/api test helpers and setup references ---\n'
rg -n --hidden 'LARKSUITE_CLI_CONFIG_DIR|TestFactory\(|newTestApiCmd|TestMain|os\.Setenv|t\.Setenv' cmd/api internal/cmdutil cmd -g '*_test.go' -g '*.go'Repository: larksuite/cli
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- cmd/api/api_test.go (top) ---'
cat -n cmd/api/api_test.go | sed -n '1,40p'
printf '\n%s\n' '--- cmd/api TestMain files ---'
fd -a 'testmain_test.go' cmd/api cmd | xargs -r -I{} sh -c 'printf "\n### %s ###\n" "{}"; cat -n "{}" | sed -n "1,120p"'
printf '\n%s\n' '--- package declarations in cmd/api ---'
rg -n '^package ' cmd/api -g '*.go'Repository: larksuite/cli
Length of output: 7281
Isolate the cmd/api tests from host config
cmd/api has no package-level TestMain, and cmdutil.TestFactory doesn’t set LARKSUITE_CLI_CONFIG_DIR. Add t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) here, or a package-level TestMain, so these tests can’t read or write a developer’s real config.
🤖 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/api/api_test.go` around lines 124 - 184, Add test isolation for the
cmd/api tests by setting LARKSUITE_CLI_CONFIG_DIR to a per-test temporary
directory via t.Setenv near the setup in TestApiCmd_UnknownFormat_Rejected,
TestApiCmd_Pretty_RejectedOnEmit, and TestApiCmd_Pretty_PreservedOnDryRun, or
establish equivalent package-wide isolation with TestMain. Ensure
cmdutil.TestFactory cannot access the developer’s real configuration.
Source: Path instructions
…ormat Address code-review findings on the emitter follow-ups: - Large pretty output is scanned in overlapping 64 KiB windows instead of one string, so content past the safety scanner's 128 KiB per-string cap is no longer skipped — this was a content-safety bypass reintroduced by the buffer-then-scan change. - Feed the canonical Format.String() into the dry-run path (api/service/ shortcut) so a mixed-case --format Pretty still renders the plain-text preview instead of falling through to the JSON envelope. - The raw api/service unknown-format error lists only json/ndjson/table/csv, not the shortcut-only pretty, so it no longer suggests a value those commands reject.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/service/service_test.go (1)
830-836: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the validation parameter, not only the message.
This test does not verify that the typed error contains
Param == "--format", so removingWithParam("--format")would still pass. Add anerrors.Asassertion for*errs.ValidationError.As per coding guidelines, command error-path tests must assert typed metadata including
param. Based on learnings,errs.ProblemOfexposes noParam; useerrors.Aswith*errs.ValidationErrorfor this field. The production path incmd/service/service.goattaches this parameter.🤖 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/service/service_test.go` around lines 830 - 836, Enhance the validation error test around requireProblem to extract the typed error with errors.As into *errs.ValidationError and assert its Param equals "--format". Keep the existing category, subtype, and message assertions, while verifying the production-attached parameter metadata directly.Sources: Coding guidelines, Learnings
🧹 Nitpick comments (1)
shortcuts/common/runner_jq_test.go (1)
355-356: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the standard isolated factory for executable shortcut tests.
Both new tests execute
runShortcutbut construct a manualcmdutil.Factory. Usecmdutil.TestFactory(t, config)and the repository’s required configuration isolation pattern so these tests do not diverge from the standard runtime setup.As per coding guidelines, test factories must use
cmdutil.TestFactory(t, config)and isolateLARKSUITE_CLI_CONFIG_DIR. Based on learnings, direct factory construction is appropriate only for mount-only tests; these tests executerunShortcut.Also applies to: 380-381
🤖 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_jq_test.go` around lines 355 - 356, Replace the manual factory setup in both tests around newTestFactory and newTestShortcutCmd with cmdutil.TestFactory(t, config), applying the repository’s standard LARKSUITE_CLI_CONFIG_DIR isolation pattern. Keep the runShortcut execution unchanged and use direct factory construction only for mount-only tests.Sources: Coding guidelines, Learnings
🤖 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/common/runner_jq_test.go`:
- Around line 385-392: Update the unknown-format test around runShortcut to
inspect the typed validation problem via errs.ProblemOf and assert
errs.CategoryValidation and errs.SubtypeInvalidArgument, while retaining
errors.As to extract Param from *errs.ValidationError. Preserve the existing
--format and “pretty” message assertions, using ParseFormatStrict’s defined
typed values.
---
Outside diff comments:
In `@cmd/service/service_test.go`:
- Around line 830-836: Enhance the validation error test around requireProblem
to extract the typed error with errors.As into *errs.ValidationError and assert
its Param equals "--format". Keep the existing category, subtype, and message
assertions, while verifying the production-attached parameter metadata directly.
---
Nitpick comments:
In `@shortcuts/common/runner_jq_test.go`:
- Around line 355-356: Replace the manual factory setup in both tests around
newTestFactory and newTestShortcutCmd with cmdutil.TestFactory(t, config),
applying the repository’s standard LARKSUITE_CLI_CONFIG_DIR isolation pattern.
Keep the runShortcut execution unchanged and use direct factory construction
only for mount-only tests.
🪄 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: 6da1fc11-4279-4390-8a71-ea25549c0584
📒 Files selected for processing (8)
cmd/api/api.gocmd/api/api_test.gocmd/service/service.gocmd/service/service_test.gointernal/output/emitter.gointernal/output/emitter_contract_test.goshortcuts/common/runner.goshortcuts/common/runner_jq_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- shortcuts/common/runner.go
- cmd/service/service.go
- cmd/api/api.go
- cmd/api/api_test.go
- internal/output/emitter_contract_test.go
- internal/output/emitter.go
Comment on lines
+385
to
+392
| err := runShortcut(cmd, f, s, false) | ||
| if err == nil { | ||
| t.Fatal("expected a validation error for unknown --format") | ||
| } | ||
| assertValidationParam(t, err, "--format") | ||
| if !strings.Contains(strings.ToLower(err.Error()), "pretty") { | ||
| t.Fatalf("shortcut unknown-format error = %v, want pretty in allowed choices", err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the complete typed validation problem.
The visible assertions check the parameter and message text, but do not assert errs.CategoryValidation and errs.SubtypeInvalidArgument. Add those checks through errs.ProblemOf; retain errors.As for Param.
As per coding guidelines, command error-path tests must verify category, subtype, and parameter. Based on learnings, Param must be read from *errs.ValidationError, not errs.ProblemOf. The ParseFormatStrict contract in internal/output/format_type.go defines the expected typed values.
🤖 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_jq_test.go` around lines 385 - 392, Update the
unknown-format test around runShortcut to inspect the typed validation problem
via errs.ProblemOf and assert errs.CategoryValidation and
errs.SubtypeInvalidArgument, while retaining errors.As to extract Param from
*errs.ValidationError. Preserve the existing --format and “pretty” message
assertions, using ParseFormatStrict’s defined typed values.
Sources: Coding guidelines, Learnings
Further review fixes: - Normalize the framework-injected --format to its canonical lowercase and write it back to the runtime context and the cobra flag, so shortcuts that branch on the exact value (e.g. Format == "pretty") behave correctly for mixed-case input like --format Pretty, which previously slipped past those checks and produced empty output. - Size the pretty rendered-text scan window to the content-safety scanner's native 128 KiB per-string capacity (was 64 KiB) so the windowing is no more restrictive than scanning the raw value; keep the 4 KiB overlap for matches crossing a window boundary.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/common/runner_jq_test.go`:
- Around line 385-386: Update the execution-path test around newTestShortcutCmd
to use cmdutil.TestFactory(t, config) instead of newTestFactory(). Isolate
LARKSUITE_CLI_CONFIG_DIR with t.Setenv and t.TempDir() before creating the
command, while preserving the existing shortcut execution assertions.
🪄 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: 3530ff2f-fe48-4e13-8d0c-0258fbbb2a3b
📒 Files selected for processing (4)
internal/output/emitter.gointernal/output/emitter_contract_test.goshortcuts/common/runner.goshortcuts/common/runner_jq_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/common/runner.go
- internal/output/emitter_contract_test.go
- internal/output/emitter.go
Comment on lines
+385
to
+386
| f := newTestFactory() | ||
| cmd := newTestShortcutCmd(s, f) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the repository test factory for this execution-path test.
runShortcut reaches configuration/auth setup and executes the shortcut, so this is not a mount-only test. Replace newTestFactory() with cmdutil.TestFactory(t, config) and isolate LARKSUITE_CLI_CONFIG_DIR with t.Setenv(..., t.TempDir()).
As per coding guidelines, *_test.go fixtures must use cmdutil.TestFactory(t, config) and isolate configuration state; the shortcut test learning limits raw factories to mount-only tests.
🤖 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_jq_test.go` around lines 385 - 386, Update the
execution-path test around newTestShortcutCmd to use cmdutil.TestFactory(t,
config) instead of newTestFactory(). Isolate LARKSUITE_CLI_CONFIG_DIR with
t.Setenv and t.TempDir() before creating the command, while preserving the
existing shortcut execution assertions.
Sources: Coding guidelines, Learnings
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
1 participant
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
Follow-ups from the PR #1899 (output Emitter convergence) owner review, each establishing one verifiable invariant. Scope kept to items ①, ②, ④ and the pagination-merge half of ⑤; items ③ (poisoned-stream state machine) and the notice-ownership half of ④ are deliberately deferred.
Changes
refactor(output)!— strict typed Format (--format(e.g. a typo like--format tabel) is now a typedValidationErrorwith a non-zero exit code instead of a stderr warning + JSON fallback.FormatgainsFormatPretty;ParseFormatStrictrejects unknown values at the api/service/shortcut boundaries (before the dry-run branch);EmitOptions.Format/StreamOptions.Formatare the typedoutput.Format(no more.String()round-trip); the rawapi/servicecommands reject--format prettyon the emit path while keeping the dry-run preview;ValidateJqFlagsclassifies the format case-insensitively so--format JSON --jqno longer mis-rejects. Shortcuts that declare their own format flag (markdown/data) keep their enum.refactor(output)— scan pretty-rendered output. The pretty path scanned the structureddataargument but rendered via an opaque closure, so a renderer printing content absent fromdatacould bypass content-safety in block mode. Pretty output is now rendered into a buffer, the safety scan runs on the actual rendered text, and stdout receives exactly what was scanned.refactor(output)— always warn on stderr when jq may drop a content-safety alert. Whether the stderr fallback warning was written used to depend on a caller-setJQSafetyWarningflag, soapi/servicewarned but shortcut commands silently lost the alert when--jqfiltered it out. The flag is removed; the Emitter always warns when jq is applied and an alert exists.refactor(output)— unify api/service pagination intoclient.PaginateToOutput.apiPaginateandservicePaginatewere near-identical; merged into one shared function, withcheckErrandmarkErrinjected. Pure refactor, byte-identical output and error semantics.Test Plan
go test -count=1 ./internal/output/ ./cmd/api/ ./cmd/service/ ./shortcuts/common/ ./shortcuts/base/ ./shortcuts/mail/ ./shortcuts/calendar/ ./shortcuts/contact/ ./internal/client/all green.go test -count=1 ./...green except thetests/cli_e2e/*packages, which fail identically on cleanmainfor environmental reasons (require a built binary / live env), unrelated to this change.lark-cli <domain> <command>flow works as expectedRelated Issues
Summary by CodeRabbit
prettyoutput format support, including plain-text dry-run previews.--formatvalidation now fails fast with typed validation errors (no silent fallback to JSON).--format prettyis rejected for normal emit, but supported for dry-run previews.--jq/--formatcompatibility and consistent JQ safety alert warning behavior.--formathandling andprettybehavior.