From 453a2fff89082c70313cdb76b00fc85003f822a7 Mon Sep 17 00:00:00 2001 From: cygnusb Date: Sat, 30 May 2026 20:28:57 +0200 Subject: [PATCH] Fix code review issues: dead code, error handling, alignment, and DRY defaults - Remove unused modes/tail.py (run_tail was never called from cli.py) - Surface config parse errors (bad YAML, invalid structure) instead of silently swallowing them; introduce ConfigFileNotFoundError subclass so only a missing default config file is allowed to be ignored - Fix --align flag being silently ignored in TUI modes by threading it through MessageLogWidget - Highlight the pre-selected stream in StreamsWidget.update_streams (active_id was accepted but never applied) - Check all poll results for GraylogAuthError in dashboard _poll, not just messages - Remove unused formatter.detect_color() - Narrow except Exception to except ValueError for JSON parse errors in client - Replace list + manual slice with deque(maxlen=60) in ThroughputWidget - Extract DEFAULT_FIELDS, DEFAULT_RANGE_SECONDS, DEFAULT_POLL_INTERVAL_MS constants to config.py and use them everywhere instead of repeated literals Co-Authored-By: Claude Sonnet 4.6 --- src/graylog_tui/cli.py | 27 +++++++--- src/graylog_tui/client.py | 8 +-- src/graylog_tui/config.py | 24 ++++++--- src/graylog_tui/formatter.py | 6 --- src/graylog_tui/modes/tail.py | 93 --------------------------------- src/graylog_tui/tui/app.py | 11 ++-- src/graylog_tui/tui/app_logs.py | 2 +- src/graylog_tui/tui/widgets.py | 33 +++++++----- tests/test_config.py | 4 +- 9 files changed, 71 insertions(+), 137 deletions(-) delete mode 100644 src/graylog_tui/modes/tail.py diff --git a/src/graylog_tui/cli.py b/src/graylog_tui/cli.py index 080889c..e6df804 100644 --- a/src/graylog_tui/cli.py +++ b/src/graylog_tui/cli.py @@ -7,7 +7,15 @@ from graylog_tui import __version__ from graylog_tui.client import GraylogAuthError, GraylogClient, GraylogError -from graylog_tui.config import ConfigError, GraylogConfig, load_config +from graylog_tui.config import ( + DEFAULT_FIELDS, + DEFAULT_POLL_INTERVAL_MS, + DEFAULT_RANGE_SECONDS, + ConfigError, + ConfigFileNotFoundError, + GraylogConfig, + load_config, +) from graylog_tui.modes.plain import run_plain app = typer.Typer(name="graylog-tui", add_completion=False, pretty_exceptions_enable=False) @@ -74,9 +82,14 @@ def main( cfg: GraylogConfig | None = None try: cfg = load_config(config_file) - except ConfigError: - if not host: - pass # will error below if host still missing + except ConfigFileNotFoundError as e: + if config_file is not None: + typer.echo(f"ERROR: {e}", err=True) + raise typer.Exit(1) from None + # Default config absent — CLI args may supply what's needed + except ConfigError as e: + typer.echo(f"ERROR: {e}", err=True) + raise typer.Exit(1) from None effective_host = host or (cfg.host if cfg else None) if not effective_host: @@ -89,7 +102,7 @@ def main( typer.echo("ERROR: username and password must be set in config file", err=True) raise typer.Exit(1) - effective_poll_ms = poll_interval or (cfg.poll_interval_ms if cfg else 1000) + effective_poll_ms = poll_interval or (cfg.poll_interval_ms if cfg else DEFAULT_POLL_INTERVAL_MS) effective_insecure = insecure or (cfg.insecure if cfg else False) client = GraylogClient( @@ -98,8 +111,8 @@ def main( password=effective_password, insecure=effective_insecure, query=cfg.query if cfg else "*", - fields=cfg.fields if cfg else "timestamp,message,source,orig_timestamp", - range_seconds=cfg.range_seconds if cfg else 300, + fields=cfg.fields if cfg else DEFAULT_FIELDS, + range_seconds=cfg.range_seconds if cfg else DEFAULT_RANGE_SECONDS, ) effective_stream_id = _resolve_stream( diff --git a/src/graylog_tui/client.py b/src/graylog_tui/client.py index e8e6cf4..aacc3ec 100644 --- a/src/graylog_tui/client.py +++ b/src/graylog_tui/client.py @@ -5,6 +5,8 @@ import httpx +from graylog_tui.config import DEFAULT_FIELDS, DEFAULT_RANGE_SECONDS + class GraylogError(Exception): pass @@ -50,8 +52,8 @@ def __init__( password: str, insecure: bool = False, query: str = "*", - fields: str = "timestamp,message,source,orig_timestamp", - range_seconds: int = 300, + fields: str = DEFAULT_FIELDS, + range_seconds: int = DEFAULT_RANGE_SECONDS, ) -> None: self.stream_id: str | None = None self._query = query @@ -92,7 +94,7 @@ def _get(self, path: str, params: dict[str, Any] | None = None) -> Any: try: return resp.json() - except Exception as e: + except ValueError as e: raise GraylogAPIError(f"Graylog returned invalid JSON: {e}") from e def fetch_streams(self) -> list[GraylogStream]: diff --git a/src/graylog_tui/config.py b/src/graylog_tui/config.py index f631d14..b010f19 100644 --- a/src/graylog_tui/config.py +++ b/src/graylog_tui/config.py @@ -7,22 +7,30 @@ DEFAULT_CONFIG_PATH = Path.home() / ".graylog_tui" +DEFAULT_FIELDS = "timestamp,message,source,orig_timestamp" +DEFAULT_RANGE_SECONDS = 300 +DEFAULT_POLL_INTERVAL_MS = 1000 + class ConfigError(Exception): pass +class ConfigFileNotFoundError(ConfigError): + pass + + @dataclass class GraylogConfig: host: str | None username: str password: str - poll_interval_ms: int = 1000 + poll_interval_ms: int = DEFAULT_POLL_INTERVAL_MS insecure: bool = False stream_title: str | None = None query: str = "*" - fields: str = "timestamp,message,source,orig_timestamp" - range_seconds: int = 300 + fields: str = DEFAULT_FIELDS + range_seconds: int = DEFAULT_RANGE_SECONDS def load_config(path: Path | None = None) -> GraylogConfig: @@ -30,7 +38,7 @@ def load_config(path: Path | None = None) -> GraylogConfig: try: raw = config_path.read_text() except FileNotFoundError: - raise ConfigError(f"Config file not found: {config_path}") from None + raise ConfigFileNotFoundError(f"Config file not found: {config_path}") from None except OSError as e: raise ConfigError(f"Cannot read config file {config_path}: {e}") from e @@ -51,10 +59,12 @@ def load_config(path: Path | None = None) -> GraylogConfig: host=str(raw_host).rstrip("/") if raw_host else None, username=str(data["username"]), password=str(data["password"]), - poll_interval_ms=int(data.get("poll-interval") or data.get("poll_interval_ms") or 1000), + poll_interval_ms=int( + data.get("poll-interval") or data.get("poll_interval_ms") or DEFAULT_POLL_INTERVAL_MS + ), insecure=bool(data.get("insecure", False)), stream_title=data.get("stream-title") or data.get("stream_title"), query=str(data.get("query", "*")), - fields=str(data.get("fields", "timestamp,message,source,orig_timestamp")), - range_seconds=int(data.get("range", 300)), + fields=str(data.get("fields", DEFAULT_FIELDS)), + range_seconds=int(data.get("range", DEFAULT_RANGE_SECONDS)), ) diff --git a/src/graylog_tui/formatter.py b/src/graylog_tui/formatter.py index 32c2a34..4c6fceb 100644 --- a/src/graylog_tui/formatter.py +++ b/src/graylog_tui/formatter.py @@ -1,16 +1,10 @@ from __future__ import annotations -import sys - from rich.text import Text from graylog_tui.client import Message -def detect_color() -> bool: - return sys.stdout.isatty() - - def plain_line(msg: Message, source_width: int = 0) -> str: source = msg.source.ljust(source_width) if source_width else msg.source return f"{msg.timestamp} - {source} - {msg.message}" diff --git a/src/graylog_tui/modes/tail.py b/src/graylog_tui/modes/tail.py deleted file mode 100644 index a2c9200..0000000 --- a/src/graylog_tui/modes/tail.py +++ /dev/null @@ -1,93 +0,0 @@ -from __future__ import annotations - -import sys -import threading -import time -from collections import deque - -import readchar -from rich.console import Console - -from graylog_tui.client import GraylogAuthError, GraylogClient, GraylogError -from graylog_tui.formatter import format_message - -PAUSE_BUFFER_MAX = 20 - - -def _interruptible_sleep(stop: threading.Event, duration: float, step: float = 0.05) -> None: - elapsed = 0.0 - while elapsed < duration and not stop.is_set(): - time.sleep(step) - elapsed += step - - -def run_tail(client: GraylogClient, poll_interval_ms: int, align: bool = False) -> None: - console = Console(stderr=False) - seen_ids: set[str] = set() - source_width = 0 - poll_s = poll_interval_ms / 1000.0 - - paused = threading.Event() - stop = threading.Event() - pause_buffer: deque[object] = deque(maxlen=PAUSE_BUFFER_MAX) - - def keyboard_thread() -> None: - while not stop.is_set(): - try: - key = readchar.readkey() - except Exception: - break - if key == " ": - if paused.is_set(): - paused.clear() - buffered = list(pause_buffer) - pause_buffer.clear() - if buffered: - console.print(f"[dim]--- {len(buffered)} buffered messages ---[/dim]") - for line in buffered: - console.print(line) - else: - paused.set() - console.print("[dim]--- paused (press SPACE to resume) ---[/dim]") - elif key in (readchar.key.ESC, "q", "\x03"): - stop.set() - break - - kb_thread = threading.Thread(target=keyboard_thread, daemon=True) - kb_thread.start() - - try: - while not stop.is_set(): - try: - messages = client.fetch_messages() - except GraylogAuthError as e: - console.print(f"[red]ERROR:[/red] {e}", highlight=False) - stop.set() - sys.exit(1) - except GraylogError as e: - console.print(f"[yellow]WARNING:[/yellow] {e}", highlight=False) - _interruptible_sleep(stop, poll_s) - continue - - new_messages = [m for m in messages if m.id not in seen_ids] - new_messages.sort(key=lambda m: m.orig_timestamp) - - if align and new_messages: - max_sw = max(len(m.source) for m in new_messages) - if max_sw > source_width: - source_width = max_sw - - for msg in new_messages: - seen_ids.add(msg.id) - rendered = format_message(msg, source_width if align else 0, color=True) - if paused.is_set(): - pause_buffer.append(rendered) - else: - console.print(rendered) - - _interruptible_sleep(stop, poll_s) - except KeyboardInterrupt: - stop.set() - sys.exit(0) - - sys.exit(0) diff --git a/src/graylog_tui/tui/app.py b/src/graylog_tui/tui/app.py index ed107d0..20cecd8 100644 --- a/src/graylog_tui/tui/app.py +++ b/src/graylog_tui/tui/app.py @@ -40,7 +40,7 @@ def compose(self) -> ComposeResult: with Horizontal(id="charts-row"): yield ThroughputWidget("Stream", color="red", id="stream-chart") yield ThroughputWidget("Total", color="green", id="total-chart") - yield MessageLogWidget(id="msg-widget") + yield MessageLogWidget(align=self._align, id="msg-widget") yield StreamsWidget(id="streams-widget") def on_mount(self) -> None: @@ -69,10 +69,11 @@ async def _poll(self) -> None: messages_result, total_tp_result, stream_tp_result = results - if isinstance(messages_result, GraylogAuthError): - self.notify("Authentication failed", severity="error", timeout=0) - self.exit() - return + for result in results: + if isinstance(result, GraylogAuthError): + self.notify("Authentication failed", severity="error", timeout=0) + self.exit() + return for result in results: if isinstance(result, GraylogError): diff --git a/src/graylog_tui/tui/app_logs.py b/src/graylog_tui/tui/app_logs.py index e4a4c69..0ddc433 100644 --- a/src/graylog_tui/tui/app_logs.py +++ b/src/graylog_tui/tui/app_logs.py @@ -30,7 +30,7 @@ def __init__(self, client: GraylogClient, poll_interval_ms: int, align: bool = F self._paused = False def compose(self) -> ComposeResult: - yield MessageLogWidget(id="msg-widget") + yield MessageLogWidget(align=self._align, id="msg-widget") def on_mount(self) -> None: widget = self.query_one(MessageLogWidget) diff --git a/src/graylog_tui/tui/widgets.py b/src/graylog_tui/tui/widgets.py index c446cc1..2b1ef70 100644 --- a/src/graylog_tui/tui/widgets.py +++ b/src/graylog_tui/tui/widgets.py @@ -1,5 +1,7 @@ from __future__ import annotations +from collections import deque + from textual.app import ComposeResult from textual.message import Message as TextualMessage from textual.widget import Widget @@ -29,24 +31,22 @@ def __init__(self, title: str, color: str = "green", **kwargs: object) -> None: super().__init__(**kwargs) # type: ignore[arg-type] self._title = title self._color = color - self._data: list[float] = [0.0] * 60 + self._data: deque[float] = deque([0.0] * 60, maxlen=60) def compose(self) -> ComposeResult: yield Label(f"{self._title}: 0.0 msg/s", id="throughput-label") - yield Sparkline(data=self._data, summary_function=max) + yield Sparkline(data=list(self._data), summary_function=max) def push_value(self, value: float) -> None: self._data.append(value) - if len(self._data) > 60: - self._data = self._data[-60:] self.query_one("#throughput-label", Label).update( f"{self._title}: {value:.1f} msg/s" ) - self.query_one(Sparkline).data = self._data + self.query_one(Sparkline).data = list(self._data) def clear(self) -> None: - self._data = [0.0] * 60 - self.query_one(Sparkline).data = self._data + self._data = deque([0.0] * 60, maxlen=60) + self.query_one(Sparkline).data = list(self._data) class StreamsWidget(Widget): @@ -81,9 +81,14 @@ def update_streams(self, streams: list[GraylogStream], active_id: str | None = N self._streams = streams lv = self.query_one("#stream-list", ListView) lv.clear() - for stream in streams: + active_index = None + for i, stream in enumerate(streams): item = ListItem(Label(stream.title[:22]), id=f"stream-{stream.id}") lv.append(item) + if stream.id == active_id: + active_index = i + if active_index is not None: + lv.index = active_index def on_list_view_selected(self, event: ListView.Selected) -> None: if event.item.id: @@ -105,8 +110,9 @@ class MessageLogWidget(Widget): } """ - def __init__(self, **kwargs: object) -> None: + def __init__(self, align: bool = True, **kwargs: object) -> None: super().__init__(**kwargs) # type: ignore[arg-type] + self._align = align self._source_width = 0 def compose(self) -> ComposeResult: @@ -115,12 +121,13 @@ def compose(self) -> ComposeResult: def add_messages(self, messages: list[GraylogMessage]) -> None: if not messages: return - max_sw = max(len(m.source) for m in messages) - if max_sw > self._source_width: - self._source_width = max_sw + if self._align: + max_sw = max(len(m.source) for m in messages) + if max_sw > self._source_width: + self._source_width = max_sw log = self.query_one("#msg-log", RichLog) for msg in messages: - rendered = format_message(msg, self._source_width, color=True) + rendered = format_message(msg, self._source_width if self._align else 0, color=True) log.write(rendered) def clear(self) -> None: diff --git a/tests/test_config.py b/tests/test_config.py index f6fa208..2b45943 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -2,7 +2,7 @@ import pytest -from graylog_tui.config import ConfigError, load_config +from graylog_tui.config import ConfigError, ConfigFileNotFoundError, load_config def write_config(tmp_path: Path, content: str) -> Path: @@ -43,7 +43,7 @@ def test_load_config_optional_fields(tmp_path: Path) -> None: def test_load_config_file_not_found() -> None: - with pytest.raises(ConfigError, match="not found"): + with pytest.raises(ConfigFileNotFoundError, match="not found"): load_config(Path("/nonexistent/path/.graylog_tui"))