fix(slides): reindent xml-get output for readability#1987
Conversation
The API always returns presentation/slide XML as a single unindented line. slides +xml-get now reformats it with etree before writing to --output, stdout, or the JSON envelope, so decks with many shapes (e.g. PPTX-imported presentations) are readable without a separate formatting step. There is no flag to disable this: the command has no purpose other than handing a human readable XML.
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFetched slide and presentation XML is formatted before being returned or written, while failed formatting falls back to the original XML and reports status. Tests cover formatting, preservation, output paths, and fallback behavior. A base record E2E workflow now skips without tenant credentials. ChangesSlide XML formatting
E2E access guard
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SlidesXMLGet
participant prettyPrintXMLOrOriginal
participant etree
SlidesXMLGet->>prettyPrintXMLOrOriginal: fetched slide or presentation XML
prettyPrintXMLOrOriginal->>etree: parse and serialize XML
etree-->>prettyPrintXMLOrOriginal: formatted XML or parse error
prettyPrintXMLOrOriginal-->>SlidesXMLGet: XML and pretty_printed status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
github-actions
Bot
added
the
size/M
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@bb10b03215de6ed72c96c980e91b65b81c644a08🧩 Skill updatenpx skills add larksuite/cli#codex/cli-xml-pretty-print -y -g |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1987 +/- ##
==========================================
+ Coverage 75.05% 75.08% +0.03%
==========================================
Files 901 901
Lines 95391 95647 +256
==========================================
+ Hits 71591 71812 +221
- Misses 18307 18328 +21
- Partials 5493 5507 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
etree's Indent() decodes and strips whitespace-only text nodes indiscriminately, which collides with the SML schema's own convention of using  /	 character references to preserve deliberate spaces/tabs inside <p> and inline formatting elements (span/strong/em/etc.) - those decode to plain whitespace indistinguishable from formatting whitespace, so the reindent was silently deleting them. Reformatting now only recurses into structural elements (presentation/slide/shape/style/...) and leaves any schema-mixed text-bearing subtree exactly as parsed. CDATA is also now preserved via ReadSettings.PreserveCData instead of being flattened into escaped text. Tests: replaced self-referential wantXML expectations (computed by calling prettyPrintXML itself) with independent golden strings, and added coverage for the specific whitespace/CDATA loss patterns plus the previously-uncovered reformat-error path through the full command. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/cli_e2e/base/base_record_batch_update_workflow_test.go (1)
110-111: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssert failure for missing record update.
When a batch update request contains a non-existent record ID (
recZZZZZZZZZZZ), the API rejects the atomic request. Asserting a successful exit code (0) and atruestdout status is incorrect and appears to have been copy-pasted from the success case. Please update these assertions to expect a command failure (e.g., a non-zero exit code andfalsestdout status).🐛 Proposed fix
- missingResult.AssertExitCode(t, 0) - missingResult.AssertStdoutStatus(t, true) + require.NotEqual(t, 0, missingResult.ExitCode, "Expected non-zero exit code for missing record update") + missingResult.AssertStdoutStatus(t, false)🤖 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 `@tests/cli_e2e/base/base_record_batch_update_workflow_test.go` around lines 110 - 111, Update the missing-record batch update assertions in the missingResult workflow to expect command failure: use a non-zero exit code and a false stdout status, matching the API’s rejection of the atomic request and leaving success-case assertions unchanged.
🧹 Nitpick comments (1)
tests/cli_e2e/base/base_record_batch_update_workflow_test.go (1)
19-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
parentTvariable.
parentTis an alias fortand is passed alongside it. IfcreateTableWithRetryrequires two*testing.Targuments (e.g., for passing both a parent test and subtest context), you can safely passtdirectly for both arguments and remove the intermediate variable.♻️ Proposed refactor
- parentT := t - ctx, cancel := context.WithTimeout(context.Background(), 4*time.Minute) - t.Cleanup(cancel) - - baseToken := createBaseWithRetry(t, ctx, "lark-cli-e2e-batch-update-"+clie2e.GenerateSuffix()) - tableID, _, _ := createTableWithRetry( - t, - parentT, + ctx, cancel := context.WithTimeout(context.Background(), 4*time.Minute) + t.Cleanup(cancel) + + baseToken := createBaseWithRetry(t, ctx, "lark-cli-e2e-batch-update-"+clie2e.GenerateSuffix()) + tableID, _, _ := createTableWithRetry( + t, + t,🤖 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 `@tests/cli_e2e/base/base_record_batch_update_workflow_test.go` around lines 19 - 26, Remove the redundant parentT alias in the batch update workflow test and pass t directly to both arguments of createTableWithRetry, preserving the existing test context and behavior.
🤖 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.
Outside diff comments:
In `@tests/cli_e2e/base/base_record_batch_update_workflow_test.go`:
- Around line 110-111: Update the missing-record batch update assertions in the
missingResult workflow to expect command failure: use a non-zero exit code and a
false stdout status, matching the API’s rejection of the atomic request and
leaving success-case assertions unchanged.
---
Nitpick comments:
In `@tests/cli_e2e/base/base_record_batch_update_workflow_test.go`:
- Around line 19-26: Remove the redundant parentT alias in the batch update
workflow test and pass t directly to both arguments of createTableWithRetry,
preserving the existing test context and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b315255f-cad1-4c25-9a24-69545fc6cf0d
📒 Files selected for processing (1)
tests/cli_e2e/base/base_record_batch_update_workflow_test.go
|
One behavior risk worth tightening: That is usually equivalent at the XML layer, but Could we either preserve/re-escape sensitive whitespace inside text-bearing subtrees, or add a regression test proving that a readback containing |
The JSON envelope now carries the server's original XML verbatim: inside a JSON string every newline is escaped anyway, so reindenting there buys no readability while inflating the payload, and passthrough restores a byte-exact read path without even parsing the content (the pretty_printed field and the fallback warning disappear from this path along with the parse). --raw stdout and --output files keep the reindented form with the fidelity machinery unchanged; pretty_printed stays in --output file metadata where formatting is still attempted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4 participants
Summary
slides +xml-getnow reindents presentation/slide XML (viaetree) before writing it to--outputfiles and--rawstdout — the text surfaces a human or line tool reads directly\n, so formatting there adds no readability while inflating the payload; passthrough also keeps that read path byte-exact without even parsing the content (nopretty_printedfield, no fallback needed on this path) /	(and CR/LF forms / , incl. hex/zero-padded spellings) are preserved lexically across the parse/write pass on the formatted surfaces--rawmeans formatted XML stdout without the JSON envelope; for the unmodified server payload, use the default envelope--raw/--output, the command returns the original server content, writes an explicit warning to stderr, and (for--output) reportspretty_printed: falsein file metadatagithub.com/beevik/etreeas a direct dependencyWhy
The API returns XML as a single unindented line, which is unreadable for decks with many shapes (for example, PPTX-imported presentations). Reformatting client-side in this read-only command keeps the change contained while preserving whitespace references that can be significant in later SML write operations.
Formatting is deliberately scoped to the file/stdout text surfaces: agents consuming the JSON envelope pay per token and gain nothing from escaped newlines, and the untouched envelope doubles as a byte-exact escape hatch that needs no fidelity machinery at all.
The fallback on the formatted surfaces remains best-effort by design: unusual or non-strict XML from the service is still retrievable, but callers are no longer left unaware that formatting was skipped.
Test plan
make unit-testgo vet ./...gofmt -l .go mod tidy(no additional changes)make build--rawhelp semantics, explicit fallback signaling for raw/file output, and verbatim envelope passthrough (well-formed and malformed content, both scopes)go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/main--raw/--output, each breaks pinned tests