refactor: consolidate packages and add shared terminal module#75
refactor: consolidate packages and add shared terminal module#75jiacai2050 merged 5 commits intomainfrom
Conversation
Consolidates packages behind a single root library to simplify import statements. Renames simargs to structargs for clarity and introduces a shared term module for styling and utilities.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the project's module structure by consolidating individual packages under a unified Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring that improves the project's structure and maintainability. Consolidating packages under a single zigcli root module, renaming simargs to the more descriptive structargs, and introducing a shared term module for terminal utilities are all excellent changes. The updates are consistently applied across the codebase, including documentation, examples, and the build system. The addition of subcommand-specific help on error in structargs is also a great feature enhancement. I have one minor suggestion for the build script to make it slightly more robust.
build.zig
Outdated
| if (b.modules.get("zigcli")) |zigcli_module| { | ||
| exe_tests.root_module.addImport("zigcli", zigcli_module); | ||
| } | ||
| if (b.modules.get("build_info")) |build_info_module| { | ||
| exe_tests.root_module.addImport("build_info", build_info_module); | ||
| } |
There was a problem hiding this comment.
The if checks for the existence of zigcli and build_info modules are a bit too defensive. Since addModules is called unconditionally at the start of the build, these modules should always be present. It would be more robust to assert their existence using .? to ensure that any build misconfiguration fails early and loudly.
Consider replacing this block with:
exe_tests.root_module.addImport("zigcli", b.modules.get("zigcli").?);
exe_tests.root_module.addImport("build_info", b.modules.get("build_info").?);There was a problem hiding this comment.
Pull request overview
Consolidates the repo’s reusable Zig modules behind a single zigcli root module, renames the CLI parser module to structargs, and introduces a shared term module for ANSI styling + terminal helpers.
Changes:
- Added
lib.zigroot module exportingpretty_table,structargs,gitignore, andterm; updated build wiring accordingly. - Moved ANSI styling + terminal-width probing into
src/mod/term.zigand refactoredpretty-tableto use it. - Renamed/updated imports across binaries, examples, and docs from
simargs/individual modules tozigcli.*; improvedstructargshelp printing for subcommand parse errors.
Reviewed changes
Copilot reviewed 16 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/mod/term.zig | Adds shared ANSI style/color primitives and tty/width helpers. |
| src/mod/structargs.zig | Adjusts help/usage formatting and prints subcommand-context help on parse errors. |
| src/mod/pretty-table.zig | Removes local color impl; uses term.Style and term.terminalWidth(). |
| src/bin/zigfetch.zig | Switches CLI parsing import from simargs to zigcli.structargs. |
| src/bin/zfetch.zig | Switches to zigcli.structargs and uses zigcli.term.isTty. |
| src/bin/tree.zig | Switches imports to zigcli.structargs and zigcli.gitignore. |
| src/bin/tcp-proxy.zig | Switches CLI parsing import to zigcli.structargs. |
| src/bin/repeat.zig | Switches CLI parsing import to zigcli.structargs. |
| src/bin/progress.zig | Switches CLI parsing import to zigcli.structargs. |
| src/bin/pretty-csv.zig | Switches to zigcli.structargs and zigcli.pretty_table. |
| src/bin/pidof.zig | Switches CLI parsing import to zigcli.structargs. |
| src/bin/night-shift.zig | Switches CLI parsing import to zigcli.structargs. |
| src/bin/loc.zig | Switches to zigcli.pretty_table, zigcli.structargs, and zigcli.gitignore. |
| src/bin/dark-mode.zig | Switches CLI parsing import to zigcli.structargs. |
| src/bin/cowsay.zig | Switches CLI parsing import to zigcli.structargs. |
| lib.zig | Introduces the single zigcli root module export surface. |
| examples/structargs-demo.zig | Updates example to use zigcli.structargs. |
| examples/pretty-table-demo.zig | Updates example to use zigcli.pretty_table and zigcli.term.Style.Color. |
| docs/content/programs/tree.org | Updates module listing from simargs.zig to structargs.zig. |
| docs/content/packages/structargs.org | Adds new package documentation for structargs. |
| docs/content/packages/simargs.org | Removes old simargs package documentation. |
| docs/content/packages/pretty-table.org | Updates docs to import via zigcli and references term.Style.Color. |
| docs/content/packages/gitignore.org | Updates docs to import via zigcli.gitignore. |
| docs/content/install.org | Updates install instructions to add only the zigcli root module. |
| build.zig | Exposes zigcli module, renames demo step, and adds test-zigcli root tests. |
| GEMINI.md | Updates module naming references (structargs). |
| CLAUDE.md | Updates build/test instructions and package naming references (structargs). |
| CHANGELOG.md | Documents breaking import change + term module + structargs help behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent refactoring that significantly improves the project's structure and maintainability. Consolidating packages under a single zigcli root, renaming simargs to structargs, and introducing a shared term module are all great steps forward. The implementation is clean and consistent across the codebase. I have a couple of minor suggestions to enhance the build script's consistency and optimize the new term module.
build.zig
Outdated
| inline for (.{ "pretty-table", "structargs", "gitignore" }) |name| { | ||
| all_tests.dependOn(buildTestStep(b, .{ .mod = name }, target)); | ||
| } |
There was a problem hiding this comment.
The new term module contains tests, but it's not included in this loop for generating individual test steps. While its tests are run as part of test-zigcli, adding it here would create a zig build test-term step. This would improve consistency with how other modules like pretty-table and structargs are handled, and make it easier to run the term module's tests in isolation.
inline for (.{ "pretty-table", "structargs", "gitignore", "term" }) |name| {
all_tests.dependOn(buildTestStep(b, .{ .mod = name }, target));
}
| pub fn writePrefix(self: Style, writer: *std.Io.Writer) !void { | ||
| if (self.bold) { | ||
| try writer.writeAll("\x1b[1m"); | ||
| } | ||
| if (self.italic) { | ||
| try writer.writeAll("\x1b[3m"); | ||
| } | ||
| if (self.fg) |fg_color| { | ||
| try writer.writeAll(fg_color.toEscapeCode()); | ||
| } | ||
| if (self.bg) |bg_color| { | ||
| try writer.writeAll(bg_color.toBgEscapeCode()); | ||
| } | ||
| } |
There was a problem hiding this comment.
This function sends a separate ANSI escape sequence for each style attribute (bold, italic, colors). It's more efficient and conventional to combine these into a single SGR escape sequence. For example, \x1b[1m\x1b[31m can be sent as \x1b[1;31m.
Refactoring this to build a single sequence before writing would reduce the number of I/O calls and align with standard terminal practices.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 30 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) *Step { | ||
| const exe_tests = b.addTest(.{ | ||
| .root_module = b.createModule(.{ | ||
| .root_source_file = b.path("lib.zig"), | ||
| .target = target, |
There was a problem hiding this comment.
buildRootTestStep creates the module without setting .optimize. Elsewhere (e.g., the executable compile step) you pass .optimize = optimize, so this test target may not respect -Doptimize=.... Consider plumbing optimize into buildRootTestStep and setting it on b.createModule for consistent build modes.
| ) *Step { | |
| const exe_tests = b.addTest(.{ | |
| .root_module = b.createModule(.{ | |
| .root_source_file = b.path("lib.zig"), | |
| .target = target, | |
| ) *Step { | |
| const optimize = b.standardOptimizeOption(.{}); | |
| const exe_tests = b.addTest(.{ | |
| .root_module = b.createModule(.{ | |
| .root_source_file = b.path("lib.zig"), | |
| .target = target, | |
| .optimize = optimize, |
| const exe_tests = b.addTest(.{ | ||
| .root_module = module, | ||
| }); | ||
| exe_tests.root_module.addImport("zigcli", b.modules.get("zigcli").?); | ||
| exe_tests.root_module.addImport("build_info", b.modules.get("build_info").?); |
There was a problem hiding this comment.
In buildTestStep, the module assigned to exe_tests.root_module is created via b.createModule a few lines above without an explicit .optimize. This can make test builds use a different optimization mode than the main executable builds (which pass .optimize = optimize). Consider plumbing optimize into buildTestStep and setting it on the module for consistent -Doptimize=... behavior.
Removes specific test steps for modules from build.zig. Use zig build test-zigcli to verify all reusable module tests. This unifies the testing process into the root module execution.
Consolidates packages behind a single root library to simplify import statements. Renames simargs to structargs for clarity and introduces a shared term module for styling and utilities.