-
Notifications
You must be signed in to change notification settings - Fork 235
CLI Colors #1006
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CLI Colors #1006
Conversation
24caeb3 to
05ee4e3
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1006 +/- ##
==========================================
+ Coverage 77.35% 80.11% +2.76%
==========================================
Files 25 28 +3
Lines 1722 2409 +687
Branches 328 457 +129
==========================================
+ Hits 1332 1930 +598
- Misses 287 356 +69
- Partials 103 123 +20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
604b533 to
53654da
Compare
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Document new features added to CLI Colors PR #1006: - New tmuxp search command with field-scoped search - Enhanced tmuxp ls with --tree, --full, --json, --ndjson - Local workspace discovery from cwd and parents - tmuxp debug-info --json for machine-readable output
Document new features added to CLI Colors PR #1006: - New tmuxp search command with field-scoped search - Enhanced tmuxp ls with --tree, --full, --json, --ndjson - Local workspace discovery from cwd and parents - tmuxp debug-info --json for machine-readable output
Code reviewFound 1 issue:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
1 similar comment
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
Code reviewFound 1 issue:
Lines 333 to 372 in 72b76f6
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
Code review (fresh)No issues found. Checked for bugs and AGENTS.md compliance. Notes: Several potential issues were identified but scored below threshold:
🤖 Generated with Claude Code |
Code Review SummaryReview completed - 5 parallel agents examined:
Issues Analyzed
ResultNo blocking issues found. All scored below 80 threshold. The PR is well-structured with proper test coverage and documentation. The output format changes are disclosed in CHANGES. |
Document new features added to CLI Colors PR #1006: - New tmuxp search command with field-scoped search - Enhanced tmuxp ls with --tree, --full, --json, --ndjson - Local workspace discovery from cwd and parents - tmuxp debug-info --json for machine-readable output
why: Color tests now belong in _internal/ since the module moved there. what: - Move test_colors.py → tests/_internal/test_colors.py - Move test_colors_formatters.py → tests/_internal/test_colors_formatters.py - Move test_cli_colors_integration.py → tests/_internal/test_colors_integration.py - Create tests/_internal/conftest.py with ANSI constants and color fixtures - Update imports to use tmuxp._internal.colors
why: tmuxp_echo was in cli/utils.py but used by workspace/finders.py, creating a reverse dependency (workspace→cli). Moving it to log.py puts it in the right architectural layer alongside other logging utilities. what: - Add tmuxp_echo function to src/tmuxp/log.py - Re-export from cli/utils.py for backward compatibility - Update workspace/finders.py to import from tmuxp.log - Fix load.py to set log level on tmuxp root logger for --log-file
why: The original type `list[str] | tuple[str, str]` was semantically wrong. `tuple[str, str]` means exactly 2 strings, not a sequence of choice items. what: - Change type from `list[str] | tuple[str, str]` to `Sequence[str | tuple[str, str]]` - This correctly describes: a sequence of items where each is str or (key, value) tuple - Update docstring to clarify parameter semantics
why: Both heading() and info() used cyan color, violating the rule that adjacent hierarchy levels should have visually distinct colors. Bold alone may not be distinguishable on some terminal/font combinations. what: - Add HEADING constant with value "bright_cyan" - Update heading() to use HEADING instead of INFO - Bright cyan is visually distinct from regular cyan while maintaining the color family cohesion for information-related text
Validate that RGB tuple values are: - Integers (raises TypeError -> UnknownStyleColor if not) - In the 0-255 range (raises ValueError -> UnknownStyleColor if not) Previously, invalid values like (256, 0, 0) or (-1, 128, 0) would produce malformed ANSI escape codes that violate the standard.
The jq example `.[] | .name` doesn't work with the JSON output format
`{"workspaces": [...]}`. Fix to `.workspaces[].name`.
The argparse directive already outputs the command description from the Python docstring, so having it manually repeated in the .md file creates duplication. Remove the redundant text.
Drop the trailing colon from example category names in help text (e.g., "Machine-readable output:" → "Machine-readable output"). Cleaner formatting for sphinx-argparse documentation output.
Category headings now use clean format without "examples:" suffix:
Before: "Field-scoped search examples:"
After: "Field-scoped search:"
- build_description() formats headings as "{heading}:" not "{heading} examples:"
- Formatter recognizes category headings within examples blocks
- Add tests for category heading colorization
- Update extract_examples_from_help() to match new format
Per AGENTS.md guidelines: "Write tests as standalone functions, not classes. Avoid class TestFoo: groupings." Converted 24 test classes across 4 files: - test_formatter.py: 4 classes → 14 functions - test_output.py: 7 classes → 23 functions - test_ls.py: 6 classes → 32 functions - test_search.py: 7 classes → 40 functions (keeping parameterized tests) All 303 CLI tests pass.
Replace bare `except Exception` with specific exception types: - First try block: `except AttributeError` for string operations on non-string plugin values - Second try block: `except (ImportError, AttributeError)` for module import failures and missing plugin class attributes This prevents catching unrelated errors like KeyboardInterrupt or MemoryError that should propagate up.
Replace bare `except Exception` with `except (yaml.YAMLError, json.JSONDecodeError, OSError)` in `_get_workspace_info()`. This ensures only expected errors from config file parsing are caught, allowing unrelated errors to propagate for proper debugging.
Replace bare `except Exception` with `except (yaml.YAMLError, json.JSONDecodeError, OSError)` in `extract_workspace_fields()`. This ensures only expected errors from config file parsing are caught, allowing unrelated errors to propagate for proper debugging.
Add `monkeypatch.delenv("TMUXP_CONFIGDIR", raising=False)` to prevent
test pollution from user environment. The fixture now properly isolates
tests from all workspace directory configuration sources.
Verify that heading() applies bright_cyan (ANSI 96) with bold when colors are enabled. Previously only the disabled case was tested. Also adds ANSI_BRIGHT_CYAN constant to conftest.py for test assertions.
Add test_compile_search_patterns_invalid_regex_raises to verify that
compile_search_patterns() raises re.error when given an invalid regex
pattern like "[invalid(".
This ensures the re.error exception handling path in command_search()
is properly tested.
Code reviewFound 1 issue:
In Lines 986 to 989 in 945a6cb
Compare with the correct implementation in ls.py: Lines 438 to 441 in 945a6cb
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Change search output to use highlight() for workspace names (L1 primary content) and info() for paths (L2 supplementary info), matching the pattern used in ls.py per CLAUDE.md CLI Color Semantics. Before: names used info() (cyan), paths used muted() (blue) After: names use highlight() (magenta+bold), paths use info() (cyan)
Summary
Port vcspull's intuitive color practices into tmuxp CLI, incorporating CPython best practices.
_colors.pymodule withColorModeenum andColorsclass--colorflag (auto/always/never) to CLI root parserNO_COLORandFORCE_COLORenvironment variables (CPython-style)load.pywith semantic colors (info, success, error, highlight, etc.)ls.pyanddebug_info.pyto use Colors classDesign
The
Colorsclass wraps tmuxp's existingstyle()function with semantic methods:success()- green, for successful operationswarning()- yellow, for warningserror()- red, for errorsinfo()- cyan, for informational messageshighlight()- magenta (bold), for important textmuted()- blue, for secondary textUsage
Test plan