fix: normalize escaped sheet range separators#207
Merged
caojie0621 merged 1 commit intomainfrom Apr 2, 2026
Merged
Conversation
Accept escaped and full-width sheet/range separators in sheets shortcuts. Normalize range parsing in the shared sheets helper so read, find, write, and append handle \!, \！, and ！ consistently. Add regression tests for separator normalization in dry-run paths.
github-actions
bot
added
domain/ccm
📝 WalkthroughWalkthroughThe changes add separator normalization to handle non-ASCII and escaped exclamation mark variants in sheet ranges. A new helper function normalizes fullwidth ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
shortcuts/sheets/helpers.go (2)
83-99: Minor redundancy: double normalization.
normalizeSheetRangeSeparatorsis called at line 85, thennormalizeSheetRangeat line 90 internally calls it again. Since the operation is idempotent this is functionally correct, but consider normalizing once and passing the result through to avoid redundant string allocations.♻️ Optional: avoid double normalization
func normalizeWriteRange(sheetID, input string, values interface{}) string { rows, cols := matrixDimensions(values) input = normalizeSheetRangeSeparators(input) if input == "" { return buildRectRange(sheetID, "A1", rows, cols) } - input = normalizeSheetRange(sheetID, input) + // Input already normalized; inline the logic from normalizeSheetRange + if !strings.Contains(input, "!") && sheetID != "" && looksLikeRelativeRange(input) { + input = sheetID + "!" + input + } rangeSheetID, subRange, ok := splitSheetRange(input)Alternatively, refactor
normalizeSheetRangeto accept a pre-normalized flag or extract the core logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/sheets/helpers.go` around lines 83 - 99, In normalizeWriteRange, avoid the redundant call to normalizeSheetRangeSeparators by removing the standalone call at the top and letting normalizeSheetRange(handle the normalization internally); update normalizeWriteRange to call normalizeSheetRange once (keeping the subsequent use of splitSheetRange, buildRectRange and singleCellRangePattern unchanged) so you don't perform duplicate string normalization and extra allocations.
101-110: Same double-normalization pattern as noted above.
looksLikeRelativeRangeat line 106 will normalize again. This is correct but redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/sheets/helpers.go` around lines 101 - 110, The function validateSheetRangeInput currently calls normalizeSheetRangeSeparators and then calls looksLikeRelativeRange which normalizes again, causing redundant work; remove the initial normalizeSheetRangeSeparators call in validateSheetRangeInput so the function uses the original input for the empty and "!" checks and relies on looksLikeRelativeRange to perform normalization internally, keeping the checks and error message behavior identical while eliminating double-normalization.shortcuts/sheets/sheet_ranges_test.go (2)
44-48: Consider usingstrconv.FormatBoolfor cleaner bool-to-string conversion.The inline map works but
strconv.FormatBool(value)is more idiomatic.♻️ Simpler bool-to-string conversion
+ "strconv"for name, value := range boolFlags { - if err := cmd.Flags().Set(name, map[bool]string{true: "true", false: "false"}[value]); err != nil { + if err := cmd.Flags().Set(name, strconv.FormatBool(value)); err != nil { t.Fatalf("Flags().Set(%q) error = %v", name, err) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/sheets/sheet_ranges_test.go` around lines 44 - 48, Replace the inline map boolean-to-string conversion in the loop that sets flag values (the for name, value := range boolFlags { ... cmd.Flags().Set(...) } block) with strconv.FormatBool(value) for clarity and idiomatic code; add/import strconv if missing and use cmd.Flags().Set(name, strconv.FormatBool(value)), keeping the existing error handling (t.Fatalf) unchanged.
52-74: Good coverage of separator variants.Consider adding edge cases for completeness: empty string, whitespace-only input, and multiple separators in a single range string.
♻️ Additional test cases
tests := []struct { name string input string want string }{ {name: "standard", input: "sheet_123!A1:B2", want: "sheet_123!A1:B2"}, {name: "escaped ascii", input: `sheet_123\!A1:B2`, want: "sheet_123!A1:B2"}, {name: "fullwidth", input: "sheet_123！A1:B2", want: "sheet_123!A1:B2"}, {name: "escaped fullwidth", input: `sheet_123\！A1:B2`, want: "sheet_123!A1:B2"}, + {name: "empty", input: "", want: ""}, + {name: "whitespace only", input: " ", want: ""}, + {name: "with leading/trailing whitespace", input: " sheet_123！A1:B2 ", want: "sheet_123!A1:B2"}, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/sheets/sheet_ranges_test.go` around lines 52 - 74, Add tests for edge cases to TestNormalizeSheetRangeSeparators: include an empty string input, a whitespace-only input (e.g., " "), and inputs containing multiple separator characters (e.g., "sheet!A1!B2" and "sheet!!A1:B2") to verify normalizeSheetRangeSeparators handles or normalizes these cases; use t.Run subtests, keep t.Parallel() and assert that the returned value matches expected normalization behavior (decide expected outcomes such as returning input unchanged, trimming whitespace, or collapsing multiple separators) referencing the normalizeSheetRangeSeparators function to locate where behavior should be validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/sheets/helpers.go`:
- Around line 83-99: In normalizeWriteRange, avoid the redundant call to
normalizeSheetRangeSeparators by removing the standalone call at the top and
letting normalizeSheetRange(handle the normalization internally); update
normalizeWriteRange to call normalizeSheetRange once (keeping the subsequent use
of splitSheetRange, buildRectRange and singleCellRangePattern unchanged) so you
don't perform duplicate string normalization and extra allocations.
- Around line 101-110: The function validateSheetRangeInput currently calls
normalizeSheetRangeSeparators and then calls looksLikeRelativeRange which
normalizes again, causing redundant work; remove the initial
normalizeSheetRangeSeparators call in validateSheetRangeInput so the function
uses the original input for the empty and "!" checks and relies on
looksLikeRelativeRange to perform normalization internally, keeping the checks
and error message behavior identical while eliminating double-normalization.
In `@shortcuts/sheets/sheet_ranges_test.go`:
- Around line 44-48: Replace the inline map boolean-to-string conversion in the
loop that sets flag values (the for name, value := range boolFlags { ...
cmd.Flags().Set(...) } block) with strconv.FormatBool(value) for clarity and
idiomatic code; add/import strconv if missing and use cmd.Flags().Set(name,
strconv.FormatBool(value)), keeping the existing error handling (t.Fatalf)
unchanged.
- Around line 52-74: Add tests for edge cases to
TestNormalizeSheetRangeSeparators: include an empty string input, a
whitespace-only input (e.g., " "), and inputs containing multiple separator
characters (e.g., "sheet!A1!B2" and "sheet!!A1:B2") to verify
normalizeSheetRangeSeparators handles or normalizes these cases; use t.Run
subtests, keep t.Parallel() and assert that the returned value matches expected
normalization behavior (decide expected outcomes such as returning input
unchanged, trimming whitespace, or collapsing multiple separators) referencing
the normalizeSheetRangeSeparators function to locate where behavior should be
validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d7bb7fe-74ee-454b-a906-d79a512ffe99
📒 Files selected for processing (2)
shortcuts/sheets/helpers.goshortcuts/sheets/sheet_ranges_test.go
Greptile SummaryThis PR fixes a usability gap in the Key changes:
Minor observation:
Confidence Score: 4/5Safe to merge; the change is additive normalisation with no impact on well-formed inputs, and the new tests verify all expected variants. The implementation is logically correct and the No files require special attention; Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["User input\n(e.g. sheet_123\\！A1:B2)"] --> B["normalizeSheetRangeSeparators\n(TrimSpace + replace \\！ \\! ！ → !)"]
B --> C{input empty?}
C -- Yes --> D["Return / default range"]
C -- No --> E["Caller-specific logic"]
E --> F["normalizeSheetRange\n(also calls normalizeSheetRangeSeparators)"]
E --> G["normalizeWriteRange\n(also calls normalizeSheetRangeSeparators)"]
E --> H["validateSheetRangeInput\n(also calls normalizeSheetRangeSeparators)"]
E --> I["splitSheetRange\n(normalizes then splits on '!')"]
E --> J["looksLikeRelativeRange\n(normalizes, then regex match)"]
F --> I
F --> J
H --> J
Reviews (1): Last reviewed commit: "fix: normalize escaped sheet range separ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@e173aeb6d0e4677669ef81468f318158c2befeed🧩 Skill updatenpx skills add larksuite/cli#fix/sheet_cli -y -g |
fangshuyu-768
approved these changes
Apr 2, 2026
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
Accept escaped and full-width sheet/range separators in sheets shortcuts. Normalize range parsing in the shared sheets helper so read, find, write, and append handle !, \！, and ！ consistently.
Add regression tests for separator normalization in dry-run paths.
Changes
Test Plan
lark-cli sheets +read/+find/+write/+appendcommand works as expectedRelated Issues
Summary by CodeRabbit