feat: add String() method to Command#12
Merged
Merged
Conversation
Returns the canonical ZapScript representation of a command, enabling round-trip parse → String() → parse. Handles quoting for args containing special characters, re-escapes control characters, and reconstructs input macro commands in their native concatenated format. Needed by zaparoo-core for ZapScript-aware allow_run matching.
|
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 due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a canonical ZapScript command serializer ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
command_string_test.go (1)
112-114: Usecmp.Diffinstead of direct equality in table assertions.Line 112 uses
got != tt.want, which gives weaker diagnostics thancmp.Diff.💡 Proposed fix
- if got != tt.want { - t.Errorf("Command.String() = %q, want %q", got, tt.want) + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("Command.String() mismatch (-want +got):\n%s", diff) }As per coding guidelines: "Use
cmp.Difffrom google/go-cmp for detailed assertion output".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@command_string_test.go` around lines 112 - 114, Replace the direct equality assertion in the TestCommandString table test that checks "if got != tt.want" for Command.String() with a cmp.Diff-based assertion: import "github.com/google/go-cmp/cmp", compute diff := cmp.Diff(tt.want, got) and if diff != "" call t.Errorf with a message that includes the diff (e.g., "Command.String() mismatch (-want +got):\n%s", diff); update the test function (TestCommandString) to use cmp.Diff and add the import to the test file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@command_string_test.go`:
- Around line 29-33: The test-case struct in the tests slice causes a
fieldalignment vet failure; reorder the struct fields so the larger type
zapscript.Command (cmd) comes before the smaller string fields to satisfy
alignment (e.g., define fields as cmd zapscript.Command, want string, name
string), update any composite literals accordingly in command_string_test.go
(the tests variable and its entries), then run the project's lint-fix task to
ensure formatting and lint checks pass.
- Around line 164-165: The round-trip comparison currently suppresses
differences by using cmpopts.IgnoreUnexported(zapscript.AdvArgs{}); remove that
option from the cmp.Diff invocation comparing script1.Cmds[0] and
script2.Cmds[0], and instead explicitly compare AdvArgs via their public Raw()
values (e.g., verify script1.Cmds[0].AdvArgs.Raw() ==
script2.Cmds[0].AdvArgs.Raw() or include that Raw() field in the cmp.Diff input)
so internal differences in zapscript.AdvArgs are observed.
In `@reader.go`:
- Around line 180-183: Advanced-arg values are written raw (using
b.WriteString(value) after writing key and SymAdvArgEq) but must be
escaped/quoted like regular args; update the serialization to call
argNeedsQuoting(value) and, if true, write the escaped/quoted form via
escapeArg(value) instead of the raw value so round-trip parsing
(parseQuotedArg/parseEscapeSeq) succeeds. Locate the block that writes key,
SymAdvArgEq, and value (the b.WriteString(string(key));
b.WriteRune(SymAdvArgEq); b.WriteString(value); return true sequence) and
replace the raw write with logic that checks argNeedsQuoting(value) and uses
escapeArg(value) when needed.
- Around line 172-185: The current code uses c.AdvArgs.Range to emit
advanced-arg pairs which is non-deterministic; fix by collecting entries during
Range into a slice (or map + keys slice), sort the keys deterministically (e.g.,
with sort.Strings on string(key)), then iterate the sorted keys to write
key/value pairs in Command.String() (referencing c.AdvArgs, IsEmpty, Range and
the String-building code around SymAdvArgStart/SymAdvArgSep/SymAdvArgEq) so the
output is canonical and repeatable.
---
Nitpick comments:
In `@command_string_test.go`:
- Around line 112-114: Replace the direct equality assertion in the
TestCommandString table test that checks "if got != tt.want" for
Command.String() with a cmp.Diff-based assertion: import
"github.com/google/go-cmp/cmp", compute diff := cmp.Diff(tt.want, got) and if
diff != "" call t.Errorf with a message that includes the diff (e.g.,
"Command.String() mismatch (-want +got):\n%s", diff); update the test function
(TestCommandString) to use cmp.Diff and add the import to the test file.
🪄 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: 977c657d-03c7-4b21-8108-9ba00d6f4f89
📒 Files selected for processing (2)
command_string_test.goreader.go
- Sort adv arg keys for deterministic output - Escape adv arg values that need quoting - Fix fieldalignment in test struct - Use _, _ = pattern for strings.Builder writes - Use cmp.Diff in test assertions - Compare AdvArgs via Raw() instead of cmpopts.IgnoreUnexported
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
command_string_test.go (1)
130-184: Trim or rewrite the inline comments to explain intent, not mechanics.Comments like “Parse the original input”, “Convert to string”, and “Compare Name and Args” restate the next line. Prefer brief rationale-level comments only.
As per coding guidelines: "Do not write comments that restate what the code does - comments should explain why, not what".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@command_string_test.go` around lines 130 - 184, The inline comments that restate code mechanics (e.g., the comments immediately before reader1.ParseScript(), str := script1.Cmds[0].String(), reader2.ParseScript(), the cmp.Diff checks for Name/Args, and the "Compare AdvArgs via public Raw() accessor" line) should be removed or rewritten as short rationale-level comments explaining why the round-trip check exists (e.g., "ensure parse→String()→parse round-trips without losing command semantics") and any non-obvious intent (e.g., why we require exactly one command). Keep all code and assertions (inputs, t.Run, NewParser, ParseScript, String(), cmp.Diff, Raw()) unchanged; only replace or delete the redundant what-level comments so remaining comments convey intent, not mechanics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@command_string_test.go`:
- Around line 28-114: The test table for zapscript.Command.String() is missing
edge cases: add cases exercising args that contain a double-quote (`"`), a
carriage return (`\r`), and advanced-arg values that require quoting/escaping
(e.g., values with comma, colon, newline, tab, or caret) so
serialization/escaping logic is validated; update the tests slice in
command_string_test.go to include zapscript.Command entries that assert expected
String() outputs for these inputs and also include AdvArgs created via
zapscript.NewAdvArgs(map[string]string{...}) where a value requires
quoting/escaping, referencing Command.String(), zapscript.Command, and
zapscript.NewAdvArgs to locate the code paths to cover.
---
Nitpick comments:
In `@command_string_test.go`:
- Around line 130-184: The inline comments that restate code mechanics (e.g.,
the comments immediately before reader1.ParseScript(), str :=
script1.Cmds[0].String(), reader2.ParseScript(), the cmp.Diff checks for
Name/Args, and the "Compare AdvArgs via public Raw() accessor" line) should be
removed or rewritten as short rationale-level comments explaining why the
round-trip check exists (e.g., "ensure parse→String()→parse round-trips without
losing command semantics") and any non-obvious intent (e.g., why we require
exactly one command). Keep all code and assertions (inputs, t.Run, NewParser,
ParseScript, String(), cmp.Diff, Raw()) unchanged; only replace or delete the
redundant what-level comments so remaining comments convey intent, not
mechanics.
🪄 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: 3bcd79b1-81e3-40f9-8f3c-2f5e8e84a624
📒 Files selected for processing (2)
command_string_test.goreader.go
🚧 Files skipped from review as they are similar to previous changes (1)
- reader.go
- Add tests for double quote, carriage return in args - Add tests for adv arg values requiring quoting (comma, colon, newline, tab, caret) - Remove redundant what-level comments in round-trip test
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
Command.String()that returns the canonical ZapScript representation\n→^n,\t→^t,^→^^)input.keyboard,input.gamepad) in concatenated formatSummary by CodeRabbit
Tests
Chores