Conversation
Reviewer's GuideImplements a new Textual-based terminal UI for inspecting Ruff configuration and rules, wires it into the existing CLI via a new Sequence diagram for launching and using the RuffSync TUI inspect commandsequenceDiagram
actor User
participant Shell
participant Script as ruff-inspect
participant CLI as ruff_sync.cli
participant App as RuffSyncApp
participant Cfg as load_local_ruff_config
participant Sys as system helpers
participant Ruff as ruff binary
User->>Shell: run `ruff-inspect` or `ruff-sync inspect`
Shell->>Script: execute console script
Script->>CLI: inspect()
CLI->>CLI: normalize sys.argv and insert inspect
CLI->>CLI: main()
CLI->>CLI: parse CLI arguments
CLI->>CLI: _resolve_upstream(args)
CLI-->>CLI: returns empty upstream tuple for inspect
CLI->>App: create RuffSyncApp(args)
CLI->>App: run()
App->>App: on_mount()
App->>Cfg: load_local_ruff_config(args.to)
Cfg-->>App: local Ruff config dict
App->>App: populate ConfigTree with config
App->>App: register themes and set default
App->>App: _prime_caches() (background)
App->>Sys: get_all_ruff_rules()
Sys->>Ruff: run `ruff rule --all --output-format json`
Ruff-->>Sys: JSON rules
Sys-->>App: list of rules
App->>Sys: get_ruff_linters()
Sys->>Ruff: run `ruff linter --output-format json`
Ruff-->>Sys: JSON linters
Sys-->>App: list of linters
App->>Sys: compute_effective_rules(all_rules, config)
Sys-->>App: rules with status
App->>App: repopulate ConfigTree with linter groups
User->>App: select node or row / open Omnibox
App->>App: handle_node_selected / handle_row_selected / handle_omnibox_result
App->>Sys: get_ruff_rule_markdown() or get_ruff_config_markdown()
Sys->>Ruff: run `ruff rule` or `ruff config`
Ruff-->>Sys: markdown text
Sys-->>App: markdown
App->>App: RuleInspector.fetch_and_display(markdown)
App-->>User: render docs in TUI
Sequence diagram for in-TUI interactions between tree, table, inspector, and system helperssequenceDiagram
participant Tree as ConfigTree
participant Table as CategoryTable
participant Insp as RuleInspector
participant App as RuffSyncApp
participant Sys as system helpers
participant Ruff as ruff binary
Tree->>App: NodeSelected(data="__rules__")
App->>App: _display_effective_rules()
App->>Sys: get_all_ruff_rules()
Sys->>Ruff: ruff rule --all --output-format json
Ruff-->>Sys: JSON rules
Sys-->>App: list of rules
App->>Sys: compute_effective_rules(all_rules, config)
Sys-->>App: rules with status
App->>Table: update_rules(effective_only)
App->>Insp: update(effective rules explanation)
Table->>App: RowSelected(rule row)
App->>App: _inspect_rule(rule_code)
App->>Sys: get_ruff_rule_markdown(rule_code)
Sys->>Ruff: ruff rule CODE
Ruff-->>Sys: markdown docs
Sys-->>App: markdown
App->>Insp: fetch_and_display(target=rule_code, is_rule=true, cached_content, rule_name, rule_status)
Tree->>App: NodeSelected(config section)
App->>Table: update_content(section data)
App->>Insp: fetch_and_display(target=section_path, is_rule=false)
App->>Sys: get_ruff_config_markdown(section_path)
Sys->>Ruff: ruff config PATH
Ruff-->>Sys: markdown docs
Sys-->>App: markdown
App->>Insp: fetch_and_display(target=section_path, is_rule=false)
Class diagram for Ruff TUI application, widgets, and screensclassDiagram
class RuffSyncApp {
+Arguments args
+dict~str, Any~ config
+list~dict~ all_rules
+list~dict~ effective_rules
+list~dict~ linters
+compose() ComposeResult
+on_mount() async
+_prime_caches() async
+_display_effective_rules() async
+handle_node_selected(event)
+handle_row_selected(event)
+_inspect_rule(rule_code str)
+_get_node_path(node) str
+action_search() void
+handle_omnibox_result(rule_code str|None) void
+action_show_legend() void
+action_copy_content() void
}
class ConfigTree {
+populate(config dict~str, Any~, has_rules bool, linters list~dict~?, effective_rules list~dict~?) void
+_is_linter_active(linter dict~str, Any~, effective_rules list~dict~) bool
+_populate_linter_nodes(parent TreeNode, linters list~dict~, effective_rules list~dict~) void
+_populate_node(parent TreeNode, data Any) void
+_expand_if_fits() void
}
class CategoryTable {
+on_mount() void
+update_content(data Any) void
+update_rules(rules list~dict~) void
}
class RuleInspector {
+show_placeholder() async
+show_context(path str, value Any) void
+fetch_and_display(target str, is_rule bool, cached_content str|None, rule_name str|None, rule_status str|None) async
}
class OmniboxScreen {
+list~dict~ all_rules
+compose() ComposeResult
+on_mount() void
+handle_input_changed(event) void
+handle_input_submitted() void
+handle_option_selected(event) void
+action_cancel() void
}
class LegendScreen {
+compose() ComposeResult
}
class ThemeModule {
}
class SystemHelpers {
+get_ruff_rule_markdown(rule_code str) async str|None
+get_ruff_config_markdown(setting_path str) async str|None
+get_all_ruff_rules() async list~dict~
+get_ruff_linters() async list~dict~
+compute_effective_rules(all_rules list~dict~, toml_config Mapping~str, Any~) list~dict~
}
RuffSyncApp --> ConfigTree : uses
RuffSyncApp --> CategoryTable : uses
RuffSyncApp --> RuleInspector : uses
RuffSyncApp --> OmniboxScreen : opens modal
RuffSyncApp --> LegendScreen : opens modal
RuffSyncApp --> SystemHelpers : calls
ConfigTree <|-- Tree
CategoryTable <|-- DataTable
RuleInspector <|-- Markdown
OmniboxScreen <|-- ModalScreen
LegendScreen <|-- ModalScreen
ThemeModule ..> RuffSyncApp : provides themes
SystemHelpers ..> RuffSyncApp : subprocess-based data
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:
- In
RuffSyncApp.on_mountyou're catching a bareExceptionand only showing a generic notification; consider at least logging the underlying exception (e.g. withLOGGER.exception) so configuration load failures are diagnosable. - The rule-code detection regex (
^[A-Z]+[0-9]+$) is duplicated in bothhandle_node_selectedandhandle_row_selected; pulling this into a single compiled constant (and possibly tightening it to the exact expected rule format) would reduce drift and make the behavior easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `RuffSyncApp.on_mount` you're catching a bare `Exception` and only showing a generic notification; consider at least logging the underlying exception (e.g. with `LOGGER.exception`) so configuration load failures are diagnosable.
- The rule-code detection regex (`^[A-Z]+[0-9]+$`) is duplicated in both `handle_node_selected` and `handle_row_selected`; pulling this into a single compiled constant (and possibly tightening it to the exact expected rule format) would reduce drift and make the behavior easier to maintain.
## Individual Comments
### Comment 1
<location path="src/ruff_sync/tui/widgets.py" line_range="74-83" />
<code_context>
+ self.add_row("Value", str(data))
+
+
+class RuleInspector(Markdown):
+ """A markdown widget for inspecting Ruff rules."""
+
+ @work(thread=True)
+ async def fetch_and_display(self, rule_code: str) -> None:
+ """Fetch and display the documentation for a rule.
+
+ Args:
+ rule_code: The Ruff rule code to fetch documentation for.
+ """
+ # Set a loading message
+ self.update(f"## Inspecting {rule_code}...\n\nFetching documentation from `ruff rule`...")
+
+ content = await get_ruff_rule_markdown(rule_code)
+
+ if content:
+ self.update(content)
+ else:
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid updating the widget directly from a threaded `@work` task and reconsider mixing `thread=True` with an async function.
Textual expects UI updates from worker threads to go via `call_from_thread` (or similar), so updating the widget directly from this worker is not thread-safe. Since `get_ruff_rule_markdown` is already async and I/O-bound, the extra thread is likely unnecessary. Either:
- make this a normal async coroutine (no `thread=True`), or
- make `fetch_and_display` synchronous with `@work(thread=True)` and use `call_from_thread` for UI updates.
Both options avoid subtle concurrency issues and better match Textual’s model.
</issue_to_address>
### Comment 2
<location path="tests/test_tui.py" line_range="33-34" />
<code_context>
+ assert app.config == {}
+
+
+@pytest.mark.asyncio
+async def test_ruff_sync_app_mount(mock_args: Arguments, tmp_path: pathlib.Path) -> None:
+ pyproject = tmp_path / "pyproject.toml"
+ pyproject.write_text(
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for the failure path in `on_mount` when loading the Ruff config fails.
Since `on_mount` has explicit error handling, please add a test that patches `load_local_ruff_config` to raise, then asserts that `app.config` is `{}`, the `ConfigTree` is present but effectively empty, and (if supported by `run_test`) that an `error`-severity notification is emitted. This will verify the TUI behavior when the config file is missing or invalid.
Suggested implementation:
```python
)
app = RuffSyncApp(mock_args)
@pytest.mark.asyncio
async def test_ruff_sync_app_mount_load_config_failure(
mock_args: Arguments,
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""on_mount should handle a failure to load the Ruff config gracefully."""
# Force load_local_ruff_config to fail to exercise the error-handling path.
monkeypatch.setattr(
"ruff_sync.tui.load_local_ruff_config",
side_effect=RuntimeError("boom"),
)
app = RuffSyncApp(mock_args)
# Run the app so that on_mount is invoked.
async with app.run_test() as _pilot:
# Let the app process startup/mount events.
# Depending on how run_test is implemented, this may be unnecessary,
# but it's harmless and keeps the test robust.
await _pilot.pause()
# When config loading fails, the app should keep the default (empty) config.
assert app.config == {}
```
To fully implement the suggested test behavior, you will likely need to:
1. **Patch the correct symbol path** for `load_local_ruff_config`:
- If `RuffSyncApp` lives in a different module, update the `monkeypatch.setattr` target accordingly, e.g.:
- `"ruff_sync.app.load_local_ruff_config"` or
- `"ruff_sync.tui.app.load_local_ruff_config"`,
whichever matches where `RuffSyncApp` references it.
2. **Assert that the ConfigTree is present and effectively empty**:
- If the app exposes the tree as a widget, add assertions inside the `async with app.run_test()` block, for example:
```python
config_tree = app.query_one(ConfigTree)
assert not list(config_tree.root.children)
```
- Or, if the tree is stored as an attribute (e.g. `app.config_tree`), assert on its internal state:
```python
assert hasattr(app, "config_tree")
assert not app.config_tree.data # or the appropriate “empty” check
```
- Adjust these checks to match the actual API of your `ConfigTree` widget.
3. **Assert that an error-severity notification is emitted**, if `run_test` or the app exposes notifications:
- For example, if `RuffSyncApp` stores notifications on `app.notifications`:
```python
errors = [n for n in app.notifications if n.severity == "error"]
assert errors
```
- Or, if `pilot` has helpers to inspect notifications/logs, use those instead.
- Update the test body in the `async with app.run_test()` block to perform these assertions right after `await _pilot.pause()`.
These additional adjustments will depend on the exact interfaces of `RuffSyncApp`, `ConfigTree`, and your notification system.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Centralize Ruff rule detection regex in `src/ruff_sync/tui/constants.py` to remove duplication. - Fix thread-safety in `RuleInspector` by using non-threaded async workers for UI updates. - Improve diagnosability of configuration failures by adding `LOGGER.exception` to `on_mount`. - Add `test_ruff_sync_app_mount_load_config_failure` to verify robust error handling. - Resolve multiple mypy type errors and ruff linting issues in TUI modules and tests. - Fix missing imports in `tests/conftest.py` that were blocking test execution.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #151 +/- ##
==========================================
+ Coverage 93.68% 93.96% +0.28%
==========================================
Files 9 9
Lines 1266 1343 +77
==========================================
+ Hits 1186 1262 +76
- Misses 80 81 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Expand the rule inspector to fill the available space and hide the data table when a rule is selected.
- Implement logic to compute rule status (Enabled/Ignored/Disabled) based on user configuration and Ruff defaults. - Add an Omnibox modal for global fuzzy-searching of all Ruff rules. - Add an "Effective Active Rules" node to the configuration tree to view the status and fix availability of every rule in a formatted table. - Cache rule metadata in the background to improve TUI responsiveness.
Use row-level color coding to indicate rule status in tables and add metadata headers with status icons to the rule inspector. Centralizes rule inspection logic and adds safety checks for Ruff CLI output parsing.
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new
inspectCLI path unconditionally importsRuffSyncApp, so in environments without the optionaltextualdependency this will raise aModuleNotFoundErrorrather than failing with the friendly message expected bytests/test_minimal_imports.sh; consider wrapping the import in atry/exceptand emitting the intended guidance about installingruff-sync[tui]before returning a non‑zero exit code. - There is a
("t", "change_theme", "Theme Picker")binding defined inRuffSyncApp.BINDINGSbut no correspondingaction_change_themeimplementation, which will result in a runtime error when the key is pressed; either implement the theme‑change handler or remove the binding.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `inspect` CLI path unconditionally imports `RuffSyncApp`, so in environments without the optional `textual` dependency this will raise a `ModuleNotFoundError` rather than failing with the friendly message expected by `tests/test_minimal_imports.sh`; consider wrapping the import in a `try/except` and emitting the intended guidance about installing `ruff-sync[tui]` before returning a non‑zero exit code.
- There is a `("t", "change_theme", "Theme Picker")` binding defined in `RuffSyncApp.BINDINGS` but no corresponding `action_change_theme` implementation, which will result in a runtime error when the key is pressed; either implement the theme‑change handler or remove the binding.
## Individual Comments
### Comment 1
<location path="src/ruff_sync/system.py" line_range="89-98" />
<code_context>
+ return []
+
+
+def compute_effective_rules(
+ all_rules: list[dict[str, Any]], toml_config: Mapping[str, Any]
+) -> list[dict[str, Any]]:
+ """Determine the status (Enabled, Ignored, Disabled) for each rule.
+
+ Args:
+ all_rules: The list of all supported rules.
+ toml_config: The local configuration dictionary.
+
+ Returns:
+ The list of rules enriched with a 'status' key.
+ """
+ lint = toml_config.get("tool", {}).get("ruff", {}).get("lint", {})
+
+ select = set(lint.get("select", [])) | set(lint.get("extend-select", []))
</code_context>
<issue_to_address>
**issue (bug_risk):** The expected shape of `toml_config` here may be inconsistent with how the TUI passes configuration around.
This function assumes a top-level `tool.ruff.lint` table. However, `ConfigTree.populate`’s docstring describes its `config` argument as an already “unwrapped” Ruff config, and that same object is passed to `compute_effective_rules` from `RuffSyncApp._prime_caches`. If `load_local_ruff_config` returns the unwrapped `[tool.ruff]` table, `toml_config.get("tool", {})` will always be `{}`, so `lint` is `{}` and the computed rule statuses won’t match the real config. Please confirm the return shape of `load_local_ruff_config` and either adjust this function to accept the unwrapped config (e.g., `toml_config["lint"]`) or re-wrap it before calling, so the tree and rules view stay consistent.
</issue_to_address>
### Comment 2
<location path="tests/test_tui.py" line_range="211-207" />
<code_context>
+ mock_run.assert_called_once()
+
+
+def test_cli_ruff_inspect_entry_point(
+ cli_run: CLIRunner, tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+ """Test that the 'ruff-inspect' entry point attempts to run the app."""
+ # Mock load_local_ruff_config where it's used in RuffSyncApp.on_mount
+ monkeypatch.setattr("ruff_sync.tui.app.load_local_ruff_config", lambda _: {})
+
+ with patch("ruff_sync.tui.app.RuffSyncApp.run", return_value=0) as mock_run:
+ # Program name 'ruff-inspect' should trigger the inspect logic
+ exit_code, _out, _err = cli_run(["--to", str(tmp_path)], entry_point="ruff-inspect")
+ assert exit_code == 0
+ mock_run.assert_called_once()
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a negative/argument-variation test for the `ruff-inspect` entry point.
The current test only covers the happy path where `ruff-inspect --to ...` runs the TUI. Since the entry point also rewrites args (inserting the `inspect` subcommand when the first arg isn’t a known command, and passing through `-h/--help/--version`), a small parametrized test that exercises:
- `entry_point="ruff-inspect"` with no args
- `entry_point="ruff-inspect"` with `['--help']`
- `entry_point="ruff-inspect"` with another subcommand, e.g. `['check', '--to', ...]`
would validate the argument rewriting and guard against regressions in this glue logic.
</issue_to_address>
### Comment 3
<location path="tests/test_themes.py" line_range="25-34" />
<code_context>
+ assert str(theme.background).upper() == "#FAFAFA"
+
+
+@pytest.mark.asyncio
+async def test_theme_picker_binding(mock_args: Arguments) -> None:
+ # Create mock config so on_mount doesn't fail
+ pyproject = mock_args.to / "pyproject.toml"
</code_context>
<issue_to_address>
**suggestion (testing):** Extend theme tests to exercise the `change_theme` action behavior, not just the binding’s presence.
Currently this only checks that `t` is bound to `change_theme`. Please also simulate the keypress via the pilot (e.g. `await pilot.press("t")`, possibly multiple times) and assert that `app.theme` actually cycles through the expected registered theme names. That way the test will catch regressions in `action_change_theme`, not just changes to the key binding.
Suggested implementation:
```python
# Check default theme
@pytest.mark.asyncio
async def test_theme_picker_binding(mock_args: Arguments) -> None:
# Create mock config so on_mount doesn't fail
pyproject = mock_args.to / "pyproject.toml"
pyproject.write_text("[tool.ruff]\n", encoding="utf-8")
app = RuffSyncApp(mock_args)
async with app.run_test() as pilot:
# Capture the initial theme name and its index in the registered themes.
initial_theme_name = app.theme.name
assert initial_theme_name in app.available_themes
initial_index = app.available_themes.index(initial_theme_name)
# Press "t" once and ensure we advanced to the next theme.
await pilot.press("t")
first_theme_name = app.theme.name
first_index = app.available_themes.index(first_theme_name)
assert first_index == (initial_index + 1) % len(app.available_themes)
# Press "t" again and ensure we advanced one more step.
await pilot.press("t")
second_theme_name = app.theme.name
second_index = app.available_themes.index(second_theme_name)
assert second_index == (first_index + 1) % len(app.available_themes)
# Now press "t" enough times to wrap all the way back to the initial theme.
remaining_presses = (len(app.available_themes) - second_index + initial_index) % len(app.available_themes)
for _ in range(remaining_presses):
await pilot.press("t")
assert app.theme.name == initial_theme_name
```
If `app.theme` does not expose a `.name` attribute, replace `app.theme.name` with the appropriate way of getting the current theme's name (for example, `str(app.theme)` or a property your app defines). If your key binding is not literally `"t"` or is defined via a Textual `Binding` collection that behaves differently, adjust the pressed key in `pilot.press("t")` to match the actual binding.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Implement theme cycling action and verify with tests. - Improve dependency checking and lazy loading for the TUI to provide better error messages. - Fix rule computation to support both wrapped and unwrapped Ruff configurations. - Enhance CLI argument handling and rewriting for the ruff-inspect entry point.
- Use a custom `DependencyError` instead of direct `sys.exit` calls in utility functions. - Centralize error logging and exit code handling in the CLI entry point. - Improve testability of dependency checks through dependency injection.
Contributor
|
Kilo59
changed the title
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 by Sourcery
Introduce a Ruff-Sync terminal UI (TUI) for inspecting Ruff configuration and rules, integrate it with the CLI, and add supporting system utilities, themes, and tests.
New Features:
ruff-inspectconsole entry point and CLI subcommand wiring to launch the TUI.Enhancements:
Build:
ruff-inspectconsole script in project metadata and bump the project version to 0.1.5.dev3.CI:
Documentation:
Tests:
Chores:
ruff_sync.tuipackage and internal constants used by the TUI.