feat(simargs): robust and flexible dynamic shell completion system#79
feat(simargs): robust and flexible dynamic shell completion system#79xihale wants to merge 1 commit intojiacai2050:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive shell completion system for the simargs module, supporting Bash, Fish, and Zsh. It introduces dynamic completion capabilities, allowing the CLI to generate context-aware suggestions through user-defined functions. Key additions include CompletionContext, logic for shell detection, and automated script generation. Feedback focuses on improving code idiomaticity by using switch statements instead of or chains, fixing a test case that bypassed the intended logic path, removing dead code in the Fish completion generator, and correcting a formatting issue to comply with the project's zig fmt standard.
|
@gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a shell completion system for Bash, Zsh, and Fish, supporting both static and dynamic suggestions. It introduces CompletionContext, shell detection logic, and a dynamic execution mode for completers. Review feedback identifies several issues regarding the robustness of generated scripts, such as missing quotes for paths with spaces and unescaped special characters in shell-specific syntax. A refactoring for extension filtering in the demo was also recommended.
src/mod/simargs.zig
Outdated
| fn writeBashCompletion(comptime Typ: type, base_name: []const u8, full_cmd: []const u8, writer: anytype) !void { | ||
| try writer.print("_{s}_completion() {{\n", .{base_name}); | ||
| try writer.writeAll(BASH_COMPLETION_PREFIX); | ||
| try writeBashOptions(Typ, writer); | ||
| try writer.writeAll("\"\n\n"); | ||
|
|
||
| // Check for options with completers | ||
| try writeBashDynamicCompleters(Typ, writer); | ||
|
|
||
| try writer.writeAll(BASH_COMPLETION_SUFFIX); | ||
| try writer.print("complete -F _{s}_completion {s} {s}\n", .{ base_name, base_name, full_cmd }); | ||
| } |
There was a problem hiding this comment.
There are two issues in the Bash completion script generation:
- Invalid Function Name: Bash function names cannot contain hyphens (
-).base_nameis derived from the program name and can contain hyphens (e.g.,my-app), which will generate an invalid function name like_my-app_completion. You should sanitizebase_nameby replacing hyphens with underscores when creating the function name. - Missing Quoting: The
base_nameandfull_cmdarguments to thecompletecommand are not quoted. This will cause issues if the program path contains spaces.
These issues will result in a broken completion script.
fn writeBashCompletion(comptime Typ: type, base_name: []const u8, full_cmd: []const u8, writer: anytype) !void {
const safe_base_name = std.mem.replacement(u8, base_name, "-", "_");
try writer.print("_{s}_completion() {{\n", .{safe_base_name});
try writer.writeAll(BASH_COMPLETION_PREFIX);
try writeBashOptions(Typ, writer);
try writer.writeAll("\"\n\n");
// Check for options with completers
try writeBashDynamicCompleters(Typ, writer);
try writer.writeAll(BASH_COMPLETION_SUFFIX);
try writer.print("complete -F _{s}_completion \"{s}\" \"{s}\"\n", .{ safe_base_name, base_name, full_cmd });
}
src/mod/simargs.zig
Outdated
| try writer.print("#compdef {s} {s}\n\n", .{ base_name, full_cmd }); | ||
| try writer.writeAll(ZSH_COMPLETION_HEADER); | ||
| try writeZshCompletion(T, base_name, base_name, writer); | ||
| try writer.print("\ncompdef _{s} {s} {s}\n", .{ base_name, base_name, full_cmd }); |
There was a problem hiding this comment.
The base_name and full_cmd arguments are not quoted in the generated compdef lines for Zsh. If the program path contains spaces or other shell metacharacters, this will lead to incorrect behavior. Please enclose these arguments in double quotes for robustness.
try writer.print("#compdef \"{s}\" \"{s}\"\n\n", .{ base_name, full_cmd });
try writer.writeAll(ZSH_COMPLETION_HEADER);
try writeZshCompletion(T, base_name, base_name, writer);
try writer.print("\ncompdef _{s} \"{s}\" \"{s}\"\n", .{ base_name, base_name, full_cmd });
src/mod/simargs.zig
Outdated
| // Handled specially | ||
| } | ||
|
|
||
| try writer.print("complete -c '{s}'", .{target}); |
There was a problem hiding this comment.
The target command path is not escaped before being placed inside single quotes. If target (which can be a file path) contains a single quote, it will result in a syntax error in the generated fish completion script. You should escape the single quotes within target.
try writer.print("complete -c '{s}'", .{std.mem.replacement(u8, target, "'", "'\\''")});
src/mod/simargs.zig
Outdated
| if (comptime @hasField(@TypeOf(Typ.__completers__), f.long_name)) { | ||
| has_completer = true; | ||
| try writer.writeAll(" -x"); | ||
| try writer.print(" -a '(\"{s}\" {s} {s} fish)'", .{ bin_to_run, DYNAMIC_COMPLETION_FLAG, f.long_name }); |
There was a problem hiding this comment.
The bin_to_run argument is not escaped before being placed inside double quotes within the fish completion script's dynamic action. If the program path contains a double quote, it will cause a syntax error in the generated script. Please escape double quotes in bin_to_run.
try writer.print(" -a '(\"{s}\" {s} {s} fish)'", .{ std.mem.replacement(u8, bin_to_run, "\"", "\\\""), DYNAMIC_COMPLETION_FLAG, f.long_name });
src/mod/simargs.zig
Outdated
| var action_written = false; | ||
| if (comptime @hasDecl(Typ, "__completers__")) { | ||
| if (comptime @hasField(@TypeOf(Typ.__completers__), f.long_name)) { | ||
| try writer.print(":{s}:{{ local -a candidates; candidates=( ${{(f)\"$(command ${{words[1]}} {s} {s} zsh)\"}} ); _describe '\\''", .{ desc, DYNAMIC_COMPLETION_FLAG, f.long_name }); |
There was a problem hiding this comment.
The desc variable is used directly in the format string for the Zsh action part (:{s}:{...}). If the description contains a colon (:) or other special characters for _arguments, it will break the completion. You should escape desc here, similar to how it's done for the main description part.
try writer.writeAll(":");
try writeZshEscaped(writer, desc);
try writer.print(":{{ local -a candidates; candidates=( ${{(f)\"$(command ${{words[1]}} {s} {s} zsh)\"}} ); _describe '\\''", .{ DYNAMIC_COMPLETION_FLAG, f.long_name });
| const is_image = std.mem.eql(u8, ext, ".png") or | ||
| std.mem.eql(u8, ext, ".jpg") or | ||
| std.mem.eql(u8, ext, ".jpeg") or | ||
| std.mem.eql(u8, ext, ".svg"); |
There was a problem hiding this comment.
The chain of or conditions for checking image extensions is a bit verbose and not easily extensible. Consider refactoring this to use a loop over a collection of extensions. This will improve readability and make it easier to add more image types in the future.
const image_extensions = [_][]const u8{ ".png", ".jpg", ".jpeg", ".svg" };
const is_image = for (image_extensions) |valid_ext| {
if (std.mem.eql(u8, ext, valid_ext)) break true;
} else false;
|
先把冲突解一下吧😄 |
|
。忘记怎么转草稿了。有空了搞,这些天忙,这次是把之前的存量推了 |
Adapted from PR jiacai2050#79 to work with the refactored project structure (simargs -> structargs, src/mod/ -> src/). Addresses all Gemini review feedback: CRITICAL: - Bash: sanitize hyphens to underscores in function names, quote args HIGH: - Test: pass baz completer directly to runCompleter instead of pre-calling it - Zsh: quote base_name and full_cmd in compdef lines - Fish: escape single quotes in target path - Fish: escape double quotes in bin_to_run - Zsh: escape desc in action part using writeZshEscaped MEDIUM: - Use switch for image extension check in demo - Use proper formatting (zig fmt clean) - Use switch for shell name detection (reverted to or chains for runtime strings) - Remove dead code (empty if block in fish completion)
|
@gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive shell completion system for the structargs module, supporting Bash, Zsh, and Fish. It enables both static and dynamic completions through a new completers field in option structs, allowing developers to provide custom completion logic. The implementation includes automatic shell detection, a CompletionContext for managing shell-specific output, and a mechanism to generate completion scripts via a dedicated flag. Additionally, the PR updates help message generation for optional fields and includes extensive tests and a demo. I have no feedback to provide.
|
还需要更广泛的验证,我看看之后实验下把 bun 相关的东西迁移过来看看可用性吧。 |
Description:
This PR introduces a major overhaul of the auto-completion system for
simargs, providing a developer experience as flexible as Go'scobrawhile leveraging Zig'scomptimecapabilities for safety and performance.Key Features & Improvements:
Flexible Multi-Type Completer API:
The
__completers__struct now supports four powerful definition styles viacomptimedispatch, catering to different complexity needs:&[_][]const u8{ "a", "b" }— Quickest way for fixed sets.fn() []const []const u8— Basic logic with no allocation.fn(Allocator) ![]const CompletionItem— For generated lists where the library handles output formatting and top-level slice cleanup.fn(CompletionContext) !void— The most flexible way. It allows:Path-Aware Shell Registration (Critical for Local Dev):
Solved the long-standing issue where shell completion only works if the binary is in
PATH.argv[0])../zig-out/bin/my-tool [TAB]works perfectly immediately after building, without manual environment setup.Enhanced Fish Compatibility:
value\tdescriptionformat.complete -ctargeting to handle quoted paths correctly.Type Safety & Validation:
Implemented
validateCompleterswhich runs at compile-time. It ensures every field in your__completers__struct actually exists in your arguments struct, turning runtime completion bugs into compile-time errors.How to use:
Verification:
Automated Tests
test "dynamic completion/different types"insimargs.zigverifying all four signature types.test "completion generation/complex"to verify registration of multiple command paths.Manual Verification
Fish Shell (Path-aware & Dynamic Logic)
Bash Shell (Dynamic Triggering)