Add configurable color support to newa list command#349
Merged
Conversation
Reviewer's GuideAdds a configurable color system for the Sequence diagram for newa list color initialization and usagesequenceDiagram
actor User
participant Shell
participant NewaCLI as NewaCLI_process
participant ListCmd as list_cmd
participant ColorUtils as color_utils
participant Colors
participant ColorConfig
participant YAMLParser as yaml_parser
User->>Shell: run newa list
Shell->>NewaCLI: invoke entrypoint
NewaCLI->>ListCmd: cmd_list(ctx, ...)
activate ListCmd
ListCmd->>ColorUtils: init_colors(ctx.settings.newa_color_config)
activate ColorUtils
ColorUtils->>ColorUtils: should_use_colors()
ColorUtils-->>ColorUtils: check NO_COLOR, FORCE_COLOR, isatty(), TERM
alt colors_disabled
ColorUtils->>Colors: disable()
Colors-->>ColorUtils: colors set to empty strings
ColorUtils-->>ListCmd: return
else colors_enabled
alt config_path_provided
ColorUtils->>ColorConfig: load_from_file(Path(color_config_path))
activate ColorConfig
ColorConfig->>YAMLParser: yaml_parser().load(file)
YAMLParser-->>ColorConfig: config_dict
ColorConfig-->>ColorUtils: ColorConfig instance
deactivate ColorConfig
ColorUtils->>Colors: set palette, state, result attributes
else no_config
ColorUtils-->>ListCmd: use default Colors
end
end
deactivate ColorUtils
loop for each state_dir
ListCmd->>Colors: read STATE_DIR or ORANGE
ListCmd->>ColorUtils: colorize_text(state_dir, color_code)
ColorUtils-->>ListCmd: colored_state_dir
ListCmd->>stdout: print colored_state_dir
loop for each event_job, jira_job, execute_job
ListCmd->>ColorUtils: colorize_text(..., event/issue/request_id colors)
ListCmd->>ColorUtils: colorize_state(state)
ListCmd->>ColorUtils: colorize_result(result)
ColorUtils-->>ListCmd: colored strings
ListCmd->>stdout: print colored output
end
end
deactivate ListCmd
Class diagram for NEWA color configuration and utilitiesclassDiagram
class Settings {
+Path newa_statedir_topdir
+bool newa_clear_on_subcommand
+str newa_color_config
+str et_url
+bool et_enable_comments
+bool et_deduplicate_releases
}
class Colors {
+str RESET
+str RED
+str GREEN
+str YELLOW
+str BLUE
+str ORANGE
+str PURPLE
+str STATE_DIR
+str EVENT
+str ISSUE
+str REQUEST_ID
+str REPORTPORTAL
+str STATE_NOT_EXECUTED
+str STATE_RUNNING
+str STATE_COMPLETE
+str STATE_ERROR
+str STATE_DEFAULT
+str RESULT_PASSED
+str RESULT_FAILED
+str RESULT_NONE
+str RESULT_CANCELLED
+str RESULT_DEFAULT
+disable() void
}
class ColorConfig {
-dict~str, dict~str, str~~ config
+ColorConfig(config_dict dict~str, dict~str, str~~)
+get_color(category str, key str) str
+load_from_file(filepath Path) ColorConfig
}
class ColorUtilsModule {
+should_use_colors() bool
+colorize(text str, color_code str) str
+colorize_state(state str) str
+colorize_result(result str) str
+colorize_text(text str, color_code str) str
+init_colors(color_config_path str) void
}
Settings --> ColorUtilsModule : uses init_colors
ColorUtilsModule --> Colors : configures
ColorUtilsModule --> ColorConfig : loads
ColorConfig --> yaml_parser : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
should_use_colorslogic allowsFORCE_COLORto enable colors even when not in a TTY, but the README currently says colors are enabled only whenFORCE_COLORis not set; please adjust the docs or behavior so the precedence and effect ofFORCE_COLORare clearly consistent. - In
ColorConfig.load_from_file, all exceptions are swallowed and the defaults are used silently; consider logging or surfacing a clear error when the YAML is malformed so users understand why their custom color config is not taking effect.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `should_use_colors` logic allows `FORCE_COLOR` to enable colors even when not in a TTY, but the README currently says colors are enabled only when `FORCE_COLOR` is not set; please adjust the docs or behavior so the precedence and effect of `FORCE_COLOR` are clearly consistent.
- In `ColorConfig.load_from_file`, all exceptions are swallowed and the defaults are used silently; consider logging or surfacing a clear error when the YAML is malformed so users understand why their custom color config is not taking effect.
## Individual Comments
### Comment 1
<location path="newa/cli/commands/list_cmd.py" line_range="150-153" />
<code_context>
jira_summary = f'- {jira_job.jira.summary}' if jira_job.jira.summary else ''
jira_action_id = jira_job.jira.action_id or 'no action id'
- _print(4, f'issue {jira_job.jira.id} ({jira_action_id}) {jira_summary}')
+ _print(
+ 4,
+ colorize_text(
+ f'issue {
+ jira_job.jira.id} ({jira_action_id}) {jira_summary}',
+ issue_color))
</code_context>
<issue_to_address>
**issue (bug_risk):** The split f-string for the issue line appears syntactically invalid and may raise at import time.
The `f'issue {` string is broken across lines so that `{` is followed by a newline before the expression, which is invalid Python and will raise a `SyntaxError` at import. Please keep the f-string on a single logical line (using parentheses if needed) or build it first and then pass it to `colorize_text`, e.g.:
```python
iissue_line = f'issue {jira_job.jira.id} ({jira_action_id}) {jira_summary}'
_print(4, colorize_text(issue_line, issue_color))
```
</issue_to_address>
### Comment 2
<location path="newa/cli/color_config.py" line_range="55-59" />
<code_context>
+ if not filepath.exists():
+ return cls()
+
+ try:
+ with open(filepath) as f:
+ config_dict = yaml_parser().load(f)
+ return cls(config_dict or {})
+ except Exception:
+ # If file is malformed, return empty config (use defaults)
+ return cls()
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Catching bare `Exception` on config load may hide actionable errors.
This `except Exception:` will hide real problems (e.g., permission issues, bad YAML, IO errors) and just fall back to defaults, making failures hard to diagnose. Please either catch the specific exceptions you expect (`OSError`, `yaml.YAMLError`, etc.) or log a warning before falling back so users can see why their config was ignored.
Suggested implementation:
```python
try:
with open(filepath) as f:
config_dict = yaml_parser().load(f)
return cls(config_dict or {})
except (OSError, yaml.YAMLError) as exc:
# If file is malformed or cannot be read, return empty config (use defaults)
logger.warning(
"Failed to load color configuration from %s: %s. Falling back to defaults.",
filepath,
exc,
)
return cls()
```
To make this compile and behave correctly, you will also need to:
1. Ensure `yaml.YAMLError` is available in this module, e.g. at the top of the file:
- `import yaml` (or adjust the exception type if you are using a different YAML library/namespace).
2. Ensure a logger is defined in this module, e.g. near the imports:
- `import logging`
- `logger = logging.getLogger(__name__)`
3. If your project uses a different logging convention (e.g., `log` instead of `logger` or a shared logger utility), adjust the `logger.warning(...)` call to match that convention.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This commit introduces a comprehensive color configuration system for the 'newa list' output, providing users with customizable color schemes while maintaining sensible defaults. Features: - Automatic terminal color detection with TTY support - Environment variable controls (NO_COLOR, FORCE_COLOR) - Customizable color schemes via YAML configuration files - Separate color configuration file support - Built-in default colors that match current color scheme Implementation: - Created color_utils.py with color detection and formatting functions - Created color_config.py for YAML-based configuration loading - Added newa_color_config setting to Settings model - Updated list_cmd.py to apply colors to all output elements - Added type annotations for mypy compatibility Color categories: - Palette colors: state_dir, event, issue, request_id, reportportal - State colors: not_executed, running, complete, error, default - Result colors: passed, failed, none, cancelled, default Configuration: Users can specify color config via: - NEWA config file: [newa] color_config = /path/to/colors.yaml - Environment variable: NEWA_COLOR_CONFIG=/path/to/colors.yaml If no color config is specified or a color is omitted from the config, built-in defaults are used, ensuring backward compatibility. Documentation: - Added comprehensive README section explaining color configuration - Included example color config file with helpful comments - Documented ANSI color code reference for user convenience All changes pass pre-commit checks (autopep8, mypy, ruff) and unit tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
kkaarreell
force-pushed
the
ks_list_color
branch
from
bf8e794 to
32147de
Compare
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.
This commit introduces a comprehensive color configuration system for the 'newa list' output, providing users with customizable color schemes while maintaining sensible defaults.
Features:
Implementation:
Color categories:
Configuration:
Users can specify color config via:
If no color config is specified or a color is omitted from the config, built-in defaults are used, ensuring backward compatibility.
Documentation:
All changes pass pre-commit checks (autopep8, mypy, ruff) and unit tests.
🤖 Generated with Claude Code
Summary by Sourcery
Add configurable ANSI color support to the
newa listcommand, with auto-detection and YAML-based theming while preserving sensible defaults and a no-color mode.New Features:
Enhancements:
Documentation: