From fd3a80bcb32b859b1e88f28d2d267cf02e1c2f86 Mon Sep 17 00:00:00 2001 From: Jan Dvorak Date: Fri, 13 Mar 2026 11:54:45 +0100 Subject: [PATCH 1/4] add pagination cap warning and improve first-run experience --- CHANGELOG.md | 11 ++++++ docs/architecture.md | 2 +- docs/modules/daemon.md | 10 ++++- docs/modules/poller.md | 5 +++ forgewatch/__main__.py | 33 ++++++++++++---- forgewatch/poller.py | 6 +++ pyproject.toml | 2 +- tests/test_main.py | 40 +++++++++++++++++-- tests/test_poller.py | 88 ++++++++++++++++++++++++++++++++++++++++++ uv.lock | 2 +- 10 files changed, 183 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5dcd52b..36e54c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,17 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.5.0] - Unreleased + +### Added + +- Pagination cap warning -- the poller now logs a warning when the page limit is reached and more results are available, suggesting the user narrow their repo filter + +### Changed + +- Improved first-run experience -- `ConfigError` is now caught and displayed as a user-friendly log message instead of a raw traceback. Missing config suggests running `forgewatch setup`, invalid config suggests checking the config file. The daemon exits cleanly with code 1 +- Logging is now initialised before config loading so that config errors are properly formatted + ## [1.4.1] - 2026-03-12 ### Fixed diff --git a/docs/architecture.md b/docs/architecture.md index 4212e4c..61c9980 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -326,7 +326,7 @@ The system is designed to be resilient to transient failures: | HTTP 401 | Raise `AuthError` immediately (bad token, no point retrying) | | HTTP 403 | Respect `Retry-After` header, retry once, then return empty | | Rate limit exhausted | Preemptively wait until reset time before making request | -| Invalid config | Raise `ConfigError` at startup (fail fast) | +| Invalid config | Catch `ConfigError`, log user-friendly message with remediation hint (`forgewatch setup` for missing config, "check your config file" for validation errors), exit with code 1 | The daemon should never crash from a transient GitHub API issue. It logs the error and continues with the next poll cycle. diff --git a/docs/modules/daemon.md b/docs/modules/daemon.md index e3e0859..3810923 100644 --- a/docs/modules/daemon.md +++ b/docs/modules/daemon.md @@ -225,8 +225,14 @@ See [cli module docs](cli.md) for the full API reference. | `-c`, `--config` | Path to a TOML config file (overrides default path resolution) | | `-v`, `--verbose` | Set log level to DEBUG (overrides `config.log_level`) | -The config is loaded before `logging.basicConfig()` so that `config.log_level` -is applied at startup. The `-v` flag overrides the config log level. +Basic logging (INFO level, or DEBUG with `-v`) is initialised **before** config +loading so that any `ConfigError` is properly formatted rather than producing a +raw traceback. If the config file is missing, the error message suggests +running `forgewatch setup`; if the config is invalid, it suggests checking the +config file. In both cases the daemon exits cleanly with code 1. + +After a successful config load, the log level is reconfigured to the value +from `config.log_level` (or DEBUG if `-v` was passed). The entry point is registered in `pyproject.toml` as `forgewatch`, so after installation it can be invoked directly: diff --git a/docs/modules/poller.md b/docs/modules/poller.md index 4aefae7..9fd13e4 100644 --- a/docs/modules/poller.md +++ b/docs/modules/poller.md @@ -269,6 +269,9 @@ using the configured `_base_url`: - **403:** Reads `Retry-After` header, sleeps, retries once - **Other:** Logs error, returns items collected so far 5. Follows pagination up to `_MAX_PAGES` pages +6. If all `_MAX_PAGES` pages were fetched and a `Link: rel="next"` header + is still present, logs a warning suggesting the user narrow their repo + filter #### `_request_with_retry()` @@ -342,6 +345,7 @@ Parses the `Link` HTTP header to extract the URL with `rel="next"`. Returns | Rate limit near exhaustion | Preemptively wait until reset before making request | | All retries exhausted (5xx) | Return last response (caller handles the status code) | | All retries exhausted (network) | Raise exception (daemon catches, preserves store state) | +| Pagination cap reached | Log warning suggesting user narrow repo filter; return items collected so far | ## Usage example @@ -395,3 +399,4 @@ Tests in `tests/test_poller.py` organized into test classes: | `TestUpdateConfig` | Field updates including base_url and max_retries | | `TestCustomBaseUrl` | Custom base URL used in HTTP requests | | `TestConfigurableRetries` | Configurable retry count, zero retries | +| `TestPaginationCapWarning` | Warning when page limit reached with more pages, no warning when all consumed | diff --git a/forgewatch/__main__.py b/forgewatch/__main__.py index 14b9955..dc5d300 100644 --- a/forgewatch/__main__.py +++ b/forgewatch/__main__.py @@ -68,21 +68,40 @@ def _run_daemon(args: argparse.Namespace) -> None: """Configure logging from parsed arguments and run the daemon.""" import asyncio # noqa: PLC0415 import logging # noqa: PLC0415 + import sys # noqa: PLC0415 from pathlib import Path # noqa: PLC0415 - from .config import load_config # noqa: PLC0415 + from .config import CONFIG_DIR, CONFIG_PATH, ConfigError, load_config # noqa: PLC0415 from .daemon import Daemon # noqa: PLC0415 - config_path = Path(args.config) if args.config else None - config = load_config(config_path) - - # CLI --verbose overrides config log_level - log_level = logging.DEBUG if args.verbose else getattr(logging, config.log_level.upper()) - + # Set up basic logging before config load so errors are properly formatted + log_level = logging.DEBUG if args.verbose else logging.INFO logging.basicConfig( level=log_level, format="%(asctime)s [%(levelname)s] %(name)s: %(message)s", ) + logger = logging.getLogger(__name__) + + config_path = Path(args.config) if args.config else None + + try: + config = load_config(config_path) + except ConfigError as exc: + if "not found" in str(exc).lower(): + logger.error( # noqa: TRY400 + "%s\n\n To get started, run: forgewatch setup\n" + " Or create config manually: mkdir -p %s && cp config.example.toml %s", + exc, + CONFIG_DIR, + CONFIG_PATH, + ) + else: + logger.error("%s\n\n Check your config file for errors.", exc) # noqa: TRY400 + sys.exit(1) + + # CLI --verbose overrides config log_level; reconfigure now that config is loaded + final_level = logging.DEBUG if args.verbose else getattr(logging, config.log_level.upper()) + logging.getLogger().setLevel(final_level) daemon = Daemon(config, config_path) diff --git a/forgewatch/poller.py b/forgewatch/poller.py index ccaa83c..042bb8e 100644 --- a/forgewatch/poller.py +++ b/forgewatch/poller.py @@ -266,6 +266,12 @@ async def _search_issues(self, query: str) -> list[dict[str, Any]]: params = None # URL from Link header includes params page += 1 + if page >= _MAX_PAGES and url: + logger.warning( + "Reached page limit (%d); results may be incomplete — consider narrowing your repo filter", + _MAX_PAGES, + ) + return all_items async def _request_with_retry( diff --git a/pyproject.toml b/pyproject.toml index 385c137..409ce1b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "forgewatch" -version = "1.4.1" +version = "1.5.0.dev" description = "Daemon that monitors GitHub PRs and exposes state over D-Bus" readme = "README.md" license = "MIT" diff --git a/tests/test_main.py b/tests/test_main.py index 5610964..246405f 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -143,16 +143,48 @@ def test_default_log_level_is_info(self) -> None: class TestMainErrorHandling: - """Config errors should propagate (clean exit via exception).""" + """Config errors should be caught and produce clean exits.""" - def test_config_error_propagates(self) -> None: + def test_missing_config_exits_with_setup_hint(self, caplog: pytest.LogCaptureFixture) -> None: + """Missing config file should exit cleanly and suggest 'forgewatch setup'.""" with ( - patch("forgewatch.config.load_config", side_effect=ConfigError("bad config")), + patch( + "forgewatch.config.load_config", side_effect=ConfigError("Config file not found: /missing/config.toml") + ), patch("sys.argv", ["forgewatch"]), - pytest.raises(ConfigError, match="bad config"), + caplog.at_level(logging.ERROR), + pytest.raises(SystemExit, match="1"), ): main() + # Should mention the setup command + assert any("forgewatch setup" in record.message for record in caplog.records) + # Should not produce a raw traceback (ConfigError is caught) + + def test_invalid_config_exits_with_check_hint(self, caplog: pytest.LogCaptureFixture) -> None: + """Invalid config should exit cleanly and suggest checking config.""" + with ( + patch("forgewatch.config.load_config", side_effect=ConfigError("github_token is required")), + patch("sys.argv", ["forgewatch"]), + caplog.at_level(logging.ERROR), + pytest.raises(SystemExit, match="1"), + ): + main() + + # Should mention checking the config file + assert any("check your config file" in record.message.lower() for record in caplog.records) + + def test_config_error_exits_code_1(self) -> None: + """Any ConfigError should result in exit code 1.""" + with ( + patch("forgewatch.config.load_config", side_effect=ConfigError("some error")), + patch("sys.argv", ["forgewatch"]), + pytest.raises(SystemExit) as exc_info, + ): + main() + + assert exc_info.value.code == 1 + def test_stop_called_even_on_start_failure(self) -> None: config = _make_config() mock_daemon = MagicMock() diff --git a/tests/test_poller.py b/tests/test_poller.py index 2dede8d..28e9179 100644 --- a/tests/test_poller.py +++ b/tests/test_poller.py @@ -2,6 +2,7 @@ from __future__ import annotations +import logging import re from datetime import UTC, datetime from typing import Any @@ -12,6 +13,7 @@ from aioresponses import CallbackResult, aioresponses from forgewatch.poller import ( + _MAX_PAGES, AuthError, GitHubClient, _parse_pr, @@ -610,3 +612,89 @@ async def test_429_returns_empty(self) -> None: assert prs == [] await client.close() + + +# --------------------------------------------------------------------------- +# GitHubClient — pagination cap warning +# --------------------------------------------------------------------------- + + +class TestPaginationCapWarning: + async def test_warns_when_page_limit_reached_with_more_pages(self, caplog: pytest.LogCaptureFixture) -> None: + """When all _MAX_PAGES pages are fetched and a next link still exists, a warning should be logged.""" + client = GitHubClient(token="tok", username="user") + await client.start() + + with aioresponses() as m: + # Set up _MAX_PAGES pages, each with a Link: next header + for page_num in range(_MAX_PAGES): + items = [_make_search_item(number=page_num * 100 + i) for i in range(1, 3)] + next_url = f"{SEARCH_URL}?q=test&page={page_num + 2}" + if page_num == 0: + m.get( + SEARCH_URL_RE, + payload=_search_response(items), + headers={"Link": f'<{next_url}>; rel="next"'}, + ) + else: + prev_url = f"{SEARCH_URL}?q=test&page={page_num + 1}" + m.get( + prev_url, + payload=_search_response(items), + headers={"Link": f'<{next_url}>; rel="next"'}, + ) + + with caplog.at_level(logging.WARNING, logger="forgewatch.poller"): + prs = await client.fetch_review_requested() + + # Should have fetched items from all pages + assert len(prs) == _MAX_PAGES * 2 + + # Warning should be logged + assert any("page limit" in record.message.lower() for record in caplog.records) + assert any(str(_MAX_PAGES) in record.message for record in caplog.records) + + await client.close() + + async def test_no_warning_when_all_pages_consumed(self, caplog: pytest.LogCaptureFixture) -> None: + """When exactly _MAX_PAGES pages are fetched and the last has no next link, no warning.""" + client = GitHubClient(token="tok", username="user") + await client.start() + + with aioresponses() as m: + # Set up _MAX_PAGES pages; the last one has no Link: next header + for page_num in range(_MAX_PAGES): + items = [_make_search_item(number=page_num * 100 + 1)] + is_last = page_num == _MAX_PAGES - 1 + + if page_num == 0: + if is_last: + m.get(SEARCH_URL_RE, payload=_search_response(items)) + else: + next_url = f"{SEARCH_URL}?q=test&page={page_num + 2}" + m.get( + SEARCH_URL_RE, + payload=_search_response(items), + headers={"Link": f'<{next_url}>; rel="next"'}, + ) + else: + current_url = f"{SEARCH_URL}?q=test&page={page_num + 1}" + if is_last: + m.get(current_url, payload=_search_response(items)) + else: + next_url = f"{SEARCH_URL}?q=test&page={page_num + 2}" + m.get( + current_url, + payload=_search_response(items), + headers={"Link": f'<{next_url}>; rel="next"'}, + ) + + with caplog.at_level(logging.WARNING, logger="forgewatch.poller"): + prs = await client.fetch_review_requested() + + assert len(prs) == _MAX_PAGES + + # No pagination warning should be logged + assert not any("page limit" in record.message.lower() for record in caplog.records) + + await client.close() diff --git a/uv.lock b/uv.lock index 223e5fb..97eb194 100644 --- a/uv.lock +++ b/uv.lock @@ -442,7 +442,7 @@ wheels = [ [[package]] name = "forgewatch" -version = "1.4.1" +version = "1.5.0.dev0" source = { editable = "." } dependencies = [ { name = "aiohttp" }, From 5e7fe11e9fc381d562078937be1dc72ba37a274e Mon Sep 17 00:00:00 2001 From: Jan Dvorak Date: Fri, 13 Mar 2026 11:56:44 +0100 Subject: [PATCH 2/4] improve config validation and add configurable indicator settings --- CHANGELOG.md | 4 + config.example.toml | 9 + docs/configuration.md | 47 ++++- docs/modules/config.md | 191 +++++++++++++++++---- docs/modules/indicator.md | 52 ++++-- forgewatch/config.py | 285 +++++++++++++++++++++++++++++-- forgewatch/indicator/__main__.py | 21 ++- forgewatch/indicator/app.py | 12 +- forgewatch/indicator/client.py | 7 +- forgewatch/indicator/window.py | 9 +- tests/test_config.py | 243 +++++++++++++++++++++++++- tests/test_indicator_app.py | 25 ++- tests/test_indicator_client.py | 16 ++ tests/test_indicator_main.py | 13 +- tests/test_indicator_window.py | 18 ++ 15 files changed, 875 insertions(+), 77 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36e54c8..cae70b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,11 +10,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Pagination cap warning -- the poller now logs a warning when the page limit is reached and more results are available, suggesting the user narrow their repo filter +- Configurable indicator settings via a new `[indicator]` TOML section -- `reconnect_interval`, `window_width`, and `max_window_height` can now be tuned without code changes +- Unknown config key warnings -- `load_config()` now logs a warning for any unrecognised top-level key, helping catch typos early ### Changed - Improved first-run experience -- `ConfigError` is now caught and displayed as a user-friendly log message instead of a raw traceback. Missing config suggests running `forgewatch setup`, invalid config suggests checking the config file. The daemon exits cleanly with code 1 - Logging is now initialised before config loading so that config errors are properly formatted +- Config validation now collects all errors and reports them in a single `ConfigError`, so users can fix every problem in one pass instead of playing whack-a-mole +- Validation error messages now include actionable hints (e.g. `ghp_` token prefix example, `GitHub recommends 300s` for poll interval, `octocat/Hello-World` for repo format) ## [1.4.1] - 2026-03-12 diff --git a/config.example.toml b/config.example.toml index 375f5ed..83cbb11 100644 --- a/config.example.toml +++ b/config.example.toml @@ -48,3 +48,12 @@ repos = [] # Use "light" for light desktop panels (dark icons on light background). # Use "dark" for dark desktop panels (light icons on dark background). # icon_theme = "light" + +# --------------------------------------------------------------------------- +# Indicator settings (system tray process) +# --------------------------------------------------------------------------- + +# [indicator] +# reconnect_interval = 10 # Seconds between reconnect attempts (default: 10) +# window_width = 400 # Popup window width in pixels (default: 400) +# max_window_height = 500 # Maximum popup window height in pixels (default: 500) diff --git a/docs/configuration.md b/docs/configuration.md index bc24c4b..b578c68 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -38,6 +38,17 @@ If no config file is found at the resolved path, a `ConfigError` is raised. | `notification_urgency` | string | No | `"normal"` | Notification urgency: `"low"`, `"normal"`, or `"critical"` | | `icon_theme` | string | No | `"light"` | Icon theme for the system tray indicator: `"light"` (dark icons for light panels) or `"dark"` (light icons for dark panels) | +### `[indicator]` section + +Settings for the system tray indicator process. These are read by the +indicator via `load_indicator_config()` and have no effect on the daemon. + +| Field | Type | Required | Default | Description | +|---|---|---|---|---| +| `reconnect_interval` | integer | No | `10` | Seconds between D-Bus reconnect attempts (minimum: 1) | +| `window_width` | integer | No | `400` | Popup window width in pixels (minimum: 200) | +| `max_window_height` | integer | No | `500` | Maximum popup window height in pixels (minimum: 200) | + ## GitHub token setup ForgeWatch needs a GitHub personal access token (PAT) to query the @@ -128,8 +139,12 @@ config file without a token and supply it via the environment instead. ## Validation rules -All validation happens at config load time. If any rule fails, a `ConfigError` -is raised with a descriptive message. +All validation happens at config load time. Validation collects **all** errors +and raises a single `ConfigError` listing every problem, so you can fix +everything in one pass. Error messages include actionable hints where possible +(e.g. example token prefix, recommended poll interval). + +Unrecognised top-level keys produce a log warning (possible typo detection). - `github_token` -- must be a non-empty string - `github_username` -- must be a non-empty string @@ -146,6 +161,12 @@ is raised with a descriptive message. - `notification_urgency` -- must be one of `low`, `normal`, `critical` (case-insensitive) - `icon_theme` -- must be one of `light`, `dark` (case-insensitive) +**`[indicator]` section:** + +- `reconnect_interval` -- must be an integer >= 1 +- `window_width` -- must be an integer >= 200 +- `max_window_height` -- must be an integer >= 200 + ## Example config A minimal configuration with only required fields: @@ -174,6 +195,11 @@ max_retries = 5 notification_threshold = 5 notification_urgency = "low" icon_theme = "light" + +[indicator] +reconnect_interval = 10 +window_width = 400 +max_window_height = 500 ``` Using environment variables instead of a token in the file: @@ -237,6 +263,15 @@ repos = [] # Use "light" for light desktop panels (dark icons on light background). # Use "dark" for dark desktop panels (light icons on dark background). # icon_theme = "light" + +# --------------------------------------------------------------------------- +# Indicator settings (system tray process) +# --------------------------------------------------------------------------- + +# [indicator] +# reconnect_interval = 10 # Seconds between reconnect attempts (default: 10) +# window_width = 400 # Popup window width in pixels (default: 400) +# max_window_height = 500 # Maximum popup window height in pixels (default: 500) ``` ## Runtime changes via SIGHUP @@ -258,7 +293,7 @@ immediately. ```python from pathlib import Path -from forgewatch.config import load_config +from forgewatch.config import load_config, load_indicator_config # Load from default path cfg = load_config() @@ -282,4 +317,10 @@ print(cfg.max_retries) # 3 print(cfg.notification_threshold) # 3 print(cfg.notification_urgency) # "normal" print(cfg.icon_theme) # "light" + +# Load indicator-specific config ([indicator] section) +ind = load_indicator_config() +print(ind.reconnect_interval) # 10 +print(ind.window_width) # 400 +print(ind.max_window_height) # 500 ``` diff --git a/docs/modules/config.md b/docs/modules/config.md index d9493a0..f6015ea 100644 --- a/docs/modules/config.md +++ b/docs/modules/config.md @@ -20,6 +20,7 @@ Internal constants (prefixed with `_`): | `_VALID_LOG_LEVELS` | `frozenset[str]` | `{"debug", "info", "warning", "error"}` | Allowed values for `log_level` | | `_VALID_URGENCIES` | `frozenset[str]` | `{"low", "normal", "critical"}` | Allowed values for `notification_urgency` | | `_VALID_ICON_THEMES` | `frozenset[str]` | `{"light", "dark"}` | Allowed values for `icon_theme` | +| `_KNOWN_KEYS` | `frozenset[str]` | *(all recognised top-level keys)* | Used by `_warn_unknown_keys()` to detect typos | ## `ConfigError` @@ -30,16 +31,19 @@ class ConfigError(Exception): ... Raised when configuration is invalid or missing. All validation failures produce a `ConfigError` with a human-readable message describing what went wrong. +Validation now collects **all** errors and raises a single `ConfigError` with +every problem listed (one per line), so the user can fix everything in one pass. + **Examples of error messages:** - `"Config file not found: /path/to/config.toml"` - `"Invalid TOML in /path/to/config.toml: ..."` -- `"github_token is required (set in config or GITHUB_TOKEN env var)"` +- `"github_token is required (set in config.toml or export GITHUB_TOKEN=ghp_...)"` - `"github_username is required"` - `"poll_interval must be an integer, got str"` -- `"poll_interval must be >= 30, got 10"` +- `"poll_interval must be >= 30, got 10 (GitHub recommends 300s for personal tokens)"` - `"repos must be a list"` -- `"Invalid repo format: 'not-valid' (expected 'owner/name')"` +- `"Invalid repo format: 'not-valid' (expected 'owner/name') — example: 'octocat/Hello-World'"` - `"log_level must be one of ['debug', 'error', 'info', 'warning'], got 'verbose'"` - `"notifications_enabled must be a boolean, got str"` - `"github_base_url must start with http:// or https://, got 'ftp://...'"` @@ -93,6 +97,25 @@ cfg = load_config() cfg.poll_interval = 60 # raises AttributeError ``` +## `IndicatorConfig` + +```python +@dataclass(frozen=True) +class IndicatorConfig: + reconnect_interval: int = 10 + window_width: int = 400 + max_window_height: int = 500 +``` + +An immutable (frozen) dataclass holding validated indicator-specific settings +from the `[indicator]` TOML section. + +| Field | Type | Default | Description | +|---|---|---|---| +| `reconnect_interval` | `int` | `10` | Seconds between D-Bus reconnect attempts (>= 1) | +| `window_width` | `int` | `400` | Popup window width in pixels (>= 200) | +| `max_window_height` | `int` | `500` | Maximum popup window height in pixels (>= 200) | + ## `load_config()` ```python @@ -120,9 +143,10 @@ A `Config` instance with all fields validated. 1. Resolves the file path via `_resolve_path(path)` 2. Reads and parses the TOML file -3. Applies `GITHUB_TOKEN` env var override (if set and non-empty) -4. Validates all fields via `_validate()` -5. Returns a `Config` instance +3. Warns about unrecognised top-level keys via `_warn_unknown_keys()` +4. Applies `GITHUB_TOKEN` env var override (if set and non-empty) +5. Validates all fields via `_validate()` (collects all errors before raising) +6. Returns a `Config` instance ### Example @@ -139,6 +163,52 @@ cfg = load_config("/etc/forgewatch/config.toml") cfg = load_config("./my-config.toml") ``` +## `load_indicator_config()` + +```python +def load_indicator_config(path: Path | str | None = None) -> IndicatorConfig: +``` + +Load indicator-specific configuration from the `[indicator]` TOML section and +return a validated `IndicatorConfig` instance. + +### Parameters + +| Parameter | Type | Default | Description | +|---|---|---|---| +| `path` | `Path \| str \| None` | `None` | Explicit config file path. If `None`, resolved via env var or default. | + +### Returns + +An `IndicatorConfig` instance with all fields validated. + +### Raises + +- `ConfigError` -- if the `[indicator]` section contains invalid values + +### Behavior + +1. Resolves the file path via `_resolve_path(path)` +2. If the file does not exist or contains invalid TOML, returns defaults +3. If the `[indicator]` section is missing, returns defaults +4. Validates all fields (collects all errors before raising) +5. Returns an `IndicatorConfig` instance + +### Example + +```python +from forgewatch.config import load_indicator_config + +# Defaults when [indicator] section is absent +cfg = load_indicator_config() +assert cfg.reconnect_interval == 10 +assert cfg.window_width == 400 +assert cfg.max_window_height == 500 + +# Custom path +cfg = load_indicator_config("/etc/forgewatch/config.toml") +``` + ## `_resolve_path()` (internal) ```python @@ -155,67 +225,110 @@ Resolves the config file path using three-tier precedence: - `ConfigError` -- if the resolved path does not exist -## Validation helpers (internal) +## `_warn_unknown_keys()` (internal) -Validation is split into reusable helper functions: +```python +def _warn_unknown_keys(raw: dict[str, object]) -> None: +``` -### `_require_str(raw, key, error_msg) -> str` +Logs a warning for each top-level key not in `_KNOWN_KEYS`. Helps users +catch typos in their config file. -Extracts a required non-empty string field. Raises `ConfigError` with the -provided message if the value is missing, empty, or not a string. +## Error-collecting validation helpers (internal) -### `_validate_bool(raw, key, *, default) -> bool` +These helpers append error messages to an `errors` list instead of raising +immediately, enabling multi-error reporting. + +### `_collect_str(raw, key, error_msg, errors) -> str` + +Extracts a required non-empty string field. Appends to `errors` if the value +is missing, empty, or not a string. + +### `_collect_bool(raw, key, *, default, errors) -> bool` -Extracts an optional boolean field with a default value. Raises `ConfigError` +Extracts an optional boolean field with a default value. Appends to `errors` if the value is present but not a boolean. -### `_validate_int_min(raw, key, *, default, minimum) -> int` +### `_collect_int_min(raw, key, *, default, minimum, errors) -> int` -Extracts an optional integer field with a minimum bound. Raises `ConfigError` +Extracts an optional integer field with a minimum bound. Appends to `errors` if the value is not an integer or is below the minimum. -### `_validate_choice(raw, key, *, default, choices) -> str` +### `_collect_choice(raw, key, *, default, choices, errors) -> str` Extracts an optional string field validated against a set of allowed values. -The value is normalised to lowercase before comparison. Raises `ConfigError` +The value is normalised to lowercase before comparison. Appends to `errors` if the value is not a string or not in the allowed set. -### `_validate_base_url(raw) -> str` +### `_collect_base_url(raw, errors) -> str` Extracts and validates the `github_base_url` field. Must start with `http://` -or `https://`. Trailing slashes are stripped. Raises `ConfigError` on invalid +or `https://`. Trailing slashes are stripped. Appends to `errors` on invalid values. -### `_validate_repos(raw) -> list[str]` +### `_collect_repos(raw, errors) -> list[str]` Extracts and validates the `repos` list. Each entry must be a string matching -the `_REPO_PATTERN` regex (`owner/name` format). Raises `ConfigError` on +the `_REPO_PATTERN` regex (`owner/name` format). Appends to `errors` on invalid values. +## Legacy validation helpers (internal) + +These raise-immediately helpers are kept for backward compatibility but are no +longer used by `_validate()`: + +### `_require_str(raw, key, error_msg) -> str` + +Extracts a required non-empty string field. Raises `ConfigError` immediately. + +### `_validate_bool(raw, key, *, default) -> bool` + +Extracts an optional boolean field. Raises `ConfigError` immediately. + +### `_validate_int_min(raw, key, *, default, minimum) -> int` + +Extracts an optional integer field with a minimum bound. Raises `ConfigError` +immediately. + +### `_validate_choice(raw, key, *, default, choices) -> str` + +Extracts an optional string field validated against allowed values. Raises +`ConfigError` immediately. + +### `_validate_base_url(raw) -> str` + +Extracts and validates `github_base_url`. Raises `ConfigError` immediately. + +### `_validate_repos(raw) -> list[str]` + +Extracts and validates the `repos` list. Raises `ConfigError` immediately. + ## `_validate()` (internal) ```python def _validate(raw: dict[str, object]) -> Config: ``` -Validates the raw TOML dict and returns a `Config` instance. Delegates to the -individual validation helpers above. +Validates the raw TOML dict and returns a `Config` instance. Collects **all** +validation errors via the `_collect_*` helpers and raises a single +`ConfigError` with every problem reported, so the user can fix everything in +one pass. ### Validation rules -1. `github_token` -- must be a `str` and non-empty (via `_require_str`) -2. `github_username` -- must be a `str` and non-empty (via `_require_str`) -3. `poll_interval` -- must be an `int` >= 30 (via `_validate_int_min`) -4. `repos` -- must be a `list` of strings matching `_REPO_PATTERN` (via `_validate_repos`) -5. `log_level` -- must be one of `_VALID_LOG_LEVELS` (via `_validate_choice`) -6. `notify_on_first_poll` -- must be a `bool` (via `_validate_bool`) -7. `notifications_enabled` -- must be a `bool` (via `_validate_bool`) -8. `dbus_enabled` -- must be a `bool` (via `_validate_bool`) -9. `github_base_url` -- must start with `http://` or `https://` (via `_validate_base_url`) -10. `max_retries` -- must be an `int` >= 0 (via `_validate_int_min`) -11. `notification_threshold` -- must be an `int` >= 1 (via `_validate_int_min`) -12. `notification_urgency` -- must be one of `_VALID_URGENCIES` (via `_validate_choice`) -13. `icon_theme` -- must be one of `_VALID_ICON_THEMES` (via `_validate_choice`) +1. `github_token` -- must be a `str` and non-empty (via `_collect_str`) +2. `github_username` -- must be a `str` and non-empty (via `_collect_str`) +3. `poll_interval` -- must be an `int` >= 30 (via `_collect_int_min`); error enhanced with GitHub recommendation +4. `repos` -- must be a `list` of strings matching `_REPO_PATTERN` (via `_collect_repos`); error includes example +5. `log_level` -- must be one of `_VALID_LOG_LEVELS` (via `_collect_choice`) +6. `notify_on_first_poll` -- must be a `bool` (via `_collect_bool`) +7. `notifications_enabled` -- must be a `bool` (via `_collect_bool`) +8. `dbus_enabled` -- must be a `bool` (via `_collect_bool`) +9. `github_base_url` -- must start with `http://` or `https://` (via `_collect_base_url`) +10. `max_retries` -- must be an `int` >= 0 (via `_collect_int_min`) +11. `notification_threshold` -- must be an `int` >= 1 (via `_collect_int_min`) +12. `notification_urgency` -- must be one of `_VALID_URGENCIES` (via `_collect_choice`) +13. `icon_theme` -- must be one of `_VALID_ICON_THEMES` (via `_collect_choice`) ## Tests @@ -231,3 +344,9 @@ Tests in `tests/test_config.py` covering: notification_urgency, icon_theme) -- defaults, valid values, invalid types, edge cases (case-insensitivity, trailing slash stripping, zero retries) - Edge cases (empty token/username strings, boundary poll_interval = 30) +- Unknown key warnings (typos logged, known keys silent, `[indicator]` section recognised) +- Multi-error collection (multiple validation failures in a single `ConfigError`, + actionable hints in error messages) +- `IndicatorConfig` via `load_indicator_config()` -- defaults, custom values, + partial values, missing file, invalid TOML, boundary validation for each field, + wrong types, multiple errors collected, frozen dataclass diff --git a/docs/modules/indicator.md b/docs/modules/indicator.md index 5583e5e..25ba7d5 100644 --- a/docs/modules/indicator.md +++ b/docs/modules/indicator.md @@ -51,10 +51,11 @@ Launch the indicator after verifying dependencies. 1. Calls `_check_dependencies()` -- exits with code 1 on failure 2. Parses `--verbose` flag for debug logging 3. Configures logging -4. Loads `config.toml` (best-effort) to read the `icon_theme` setting; falls - back to `"light"` if config loading fails for any reason +4. Loads `config.toml` (best-effort) to read the `icon_theme` setting and the + `[indicator]` section (`IndicatorConfig`); falls back to defaults if config + loading fails for any reason 5. Installs `gbulb` event loop (`gbulb.install()`) -6. Creates an `IndicatorApp(icon_theme=...)` and runs it in a new event loop +6. Creates an `IndicatorApp(icon_theme=..., reconnect_interval=..., window_width=..., max_window_height=...)` and runs it in a new event loop 7. Calls `app.shutdown()` in a `finally` block for clean resource release **CLI arguments:** @@ -166,6 +167,8 @@ class DaemonClient: self, on_prs_changed: Callable[[list[PRInfo]], None], on_connection_changed: Callable[[bool], None], + *, + reconnect_interval: int = _RECONNECT_INTERVAL_S, ) -> None: ... ``` @@ -175,10 +178,11 @@ bus, obtains a proxy for the daemon's interface, subscribes to the **Constructor parameters:** -| Parameter | Type | Description | -|---|---|---| -| `on_prs_changed` | `Callable[[list[PRInfo]], None]` | Called when the daemon emits `PullRequestsChanged` | -| `on_connection_changed` | `Callable[[bool], None]` | Called when connection state changes (`True` = connected) | +| Parameter | Type | Default | Description | +|---|---|---|---| +| `on_prs_changed` | `Callable[[list[PRInfo]], None]` | *(required)* | Called when the daemon emits `PullRequestsChanged` | +| `on_connection_changed` | `Callable[[bool], None]` | *(required)* | Called when connection state changes (`True` = connected) | +| `reconnect_interval` | `int` | `10` | Seconds between reconnection attempts (keyword-only) | #### Properties @@ -251,14 +255,27 @@ Returns the updated PR list, or an empty list on failure. ```python class IndicatorApp: - def __init__(self, *, icon_theme: str = "light") -> None: ... + def __init__( + self, + *, + icon_theme: str = "light", + reconnect_interval: int = 10, + window_width: int = 400, + max_window_height: int = 500, + ) -> None: ... ``` Main application that bridges D-Bus, tray icon, and popup window. Creates and wires together a `DaemonClient`, `TrayIcon`, and `PRWindow`. -The `icon_theme` parameter is forwarded to `TrayIcon` to select the icon -variant directory (`resources/light/` or `resources/dark/`). +**Constructor parameters:** + +| Parameter | Type | Default | Description | +|---|---|---|---| +| `icon_theme` | `str` | `"light"` | Forwarded to `TrayIcon` to select the icon variant directory | +| `reconnect_interval` | `int` | `10` | Forwarded to `DaemonClient` -- seconds between reconnect attempts | +| `window_width` | `int` | `400` | Forwarded to `PRWindow` -- popup window width in pixels | +| `max_window_height` | `int` | `500` | Forwarded to `PRWindow` -- maximum popup window height in pixels | The lifecycle is: 1. `run()` -- register signal handlers, connect to daemon, enter event loop @@ -399,6 +416,9 @@ class PRWindow: on_pr_clicked: Callable[[str], None], on_refresh: Callable[[], None], on_visibility_changed: Callable[[bool], None] | None = None, + *, + window_width: int = _WINDOW_WIDTH, + max_window_height: int = _MAX_WINDOW_HEIGHT, ) -> None: ... ``` @@ -412,6 +432,8 @@ list, and a status footer. | `on_pr_clicked` | `Callable[[str], None]` | *(required)* | Called with the PR URL when a row is clicked | | `on_refresh` | `Callable[[], None]` | *(required)* | Called when the header refresh button is pressed | | `on_visibility_changed` | `Callable[[bool], None] \| None` | `None` | Called when the window is shown or hidden | +| `window_width` | `int` | `400` | Popup window width in pixels (keyword-only) | +| `max_window_height` | `int` | `500` | Maximum popup window height in pixels (keyword-only) | #### Properties @@ -660,7 +682,12 @@ import gbulb from forgewatch.indicator.app import IndicatorApp gbulb.install() -app = IndicatorApp(icon_theme="dark") # or "light" (default) +app = IndicatorApp( + icon_theme="dark", + reconnect_interval=30, # retry D-Bus connection every 30s + window_width=500, # wider popup window + max_window_height=600, # taller popup window +) loop = asyncio.new_event_loop() try: loop.run_until_complete(app.run()) @@ -686,7 +713,8 @@ finally: async coroutines by wrapping them in `asyncio.ensure_future()` and storing task references to prevent garbage collection - Auto-reconnection: when the daemon exits or crashes, the client detects the - `NameOwnerChanged` signal and retries the connection every 10 seconds + `NameOwnerChanged` signal and retries the connection at a configurable + interval (default: 10 seconds, set via `reconnect_interval`) - D-Bus coordinates (`BUS_NAME`, `OBJECT_PATH`, `INTERFACE_NAME`) are duplicated from `dbus_service.py` rather than imported, reinforcing the process boundary diff --git a/forgewatch/config.py b/forgewatch/config.py index 9448692..d8c4313 100644 --- a/forgewatch/config.py +++ b/forgewatch/config.py @@ -2,6 +2,7 @@ from __future__ import annotations +import logging import os import re import tomllib @@ -18,6 +19,28 @@ _VALID_URGENCIES = frozenset({"low", "normal", "critical"}) _VALID_ICON_THEMES = frozenset({"light", "dark"}) +_KNOWN_KEYS = frozenset( + { + "github_token", + "github_username", + "poll_interval", + "repos", + "log_level", + "notify_on_first_poll", + "notifications_enabled", + "dbus_enabled", + "github_base_url", + "max_retries", + "notification_threshold", + "notification_urgency", + "icon_theme", + "indicator", + "notifications", + } +) + +logger = logging.getLogger(__name__) + class ConfigError(Exception): """Raised when configuration is invalid or cannot be loaded.""" @@ -42,6 +65,15 @@ class Config: icon_theme: str = "light" +@dataclass(frozen=True) +class IndicatorConfig: + """Configuration for the indicator process.""" + + reconnect_interval: int = 10 + window_width: int = 400 + max_window_height: int = 500 + + def load_config(path: Path | str | None = None) -> Config: """Load and validate config from TOML file. @@ -66,6 +98,9 @@ def load_config(path: Path | str | None = None) -> Config: msg = f"Invalid TOML in {config_path}: {exc}" raise ConfigError(msg) from exc + # Warn about unknown top-level keys (possible typos). + _warn_unknown_keys(raw) + # Env var override for token env_token = os.environ.get("GITHUB_TOKEN") if env_token: @@ -74,6 +109,62 @@ def load_config(path: Path | str | None = None) -> Config: return _validate(raw) +def load_indicator_config(path: Path | str | None = None) -> IndicatorConfig: + """Load indicator-specific config from the ``[indicator]`` TOML section. + + Returns ``IndicatorConfig`` with defaults if the section is missing + or the file cannot be read. Validation errors raise ``ConfigError``. + """ + config_path = _resolve_path(path) + + if not config_path.exists(): + return IndicatorConfig() + + try: + with config_path.open("rb") as f: + raw = tomllib.load(f) + except tomllib.TOMLDecodeError: + return IndicatorConfig() + + section = raw.get("indicator") + if not isinstance(section, dict): + return IndicatorConfig() + + errors: list[str] = [] + + reconnect_interval = _collect_int_min( + section, + "reconnect_interval", + default=10, + minimum=1, + errors=errors, + ) + window_width = _collect_int_min( + section, + "window_width", + default=400, + minimum=200, + errors=errors, + ) + max_window_height = _collect_int_min( + section, + "max_window_height", + default=500, + minimum=200, + errors=errors, + ) + + if errors: + msg = "\n".join(errors) + raise ConfigError(msg) + + return IndicatorConfig( + reconnect_interval=reconnect_interval, + window_width=window_width, + max_window_height=max_window_height, + ) + + def _resolve_path(path: Path | str | None) -> Path: """Determine which config file path to use.""" if path is not None: @@ -86,6 +177,124 @@ def _resolve_path(path: Path | str | None) -> Path: return CONFIG_PATH +def _warn_unknown_keys(raw: dict[str, object]) -> None: + """Log warnings for any unrecognised top-level config keys.""" + unknown = set(raw.keys()) - _KNOWN_KEYS + for key in sorted(unknown): + logger.warning("Unknown config key: %r (possible typo?)", key) + + +# --------------------------------------------------------------------------- +# Error-collecting validation helpers +# --------------------------------------------------------------------------- + + +def _collect_str( + raw: dict[str, object], + key: str, + error_msg: str, + errors: list[str], +) -> str: + """Extract a required non-empty string, appending to *errors* on failure.""" + value = raw.get(key, "") + if not value or not isinstance(value, str): + errors.append(error_msg) + return "" + return value + + +def _collect_bool( + raw: dict[str, object], + key: str, + *, + default: bool, + errors: list[str], +) -> bool: + """Extract and validate an optional boolean field, collecting errors.""" + value = raw.get(key, default) + if not isinstance(value, bool): + errors.append(f"{key} must be a boolean, got {type(value).__name__}") + return default + return value + + +def _collect_int_min( + raw: dict[str, object], + key: str, + *, + default: int, + minimum: int, + errors: list[str], +) -> int: + """Extract and validate an optional integer field with a minimum bound, collecting errors.""" + value = raw.get(key, default) + if not isinstance(value, int): + errors.append(f"{key} must be an integer, got {type(value).__name__}") + return default + if value < minimum: + errors.append(f"{key} must be >= {minimum}, got {value}") + return default + return value + + +def _collect_choice( + raw: dict[str, object], + key: str, + *, + default: str, + choices: frozenset[str], + errors: list[str], +) -> str: + """Extract and validate an optional string field against allowed values, collecting errors.""" + value = raw.get(key, default) + if not isinstance(value, str): + errors.append(f"{key} must be a string, got {type(value).__name__}") + return default + normalised = value.lower() + if normalised not in choices: + errors.append(f"{key} must be one of {sorted(choices)}, got {normalised!r}") + return default + return normalised + + +def _collect_base_url( + raw: dict[str, object], + errors: list[str], +) -> str: + """Extract and validate the GitHub base URL, collecting errors.""" + value = raw.get("github_base_url", "https://api.github.com") + if not isinstance(value, str): + errors.append(f"github_base_url must be a string, got {type(value).__name__}") + return "https://api.github.com" + if not value.startswith(("http://", "https://")): + errors.append(f"github_base_url must start with http:// or https://, got {value!r}") + return "https://api.github.com" + return value.rstrip("/") + + +def _collect_repos( + raw: dict[str, object], + errors: list[str], +) -> list[str]: + """Extract and validate the repos list, collecting errors.""" + repos = raw.get("repos", []) + if not isinstance(repos, list): + errors.append("repos must be a list") + return [] + has_error = False + for repo in repos: + if not isinstance(repo, str) or not _REPO_PATTERN.match(repo): + errors.append(f"Invalid repo format: {repo!r} (expected 'owner/name') — example: 'octocat/Hello-World'") + has_error = True + return [] if has_error else repos + + +# --------------------------------------------------------------------------- +# Legacy raise-immediately helpers (kept for backward-compat error messages +# in tests that call them directly — but _validate() no longer uses them) +# --------------------------------------------------------------------------- + + def _require_str(raw: dict[str, object], key: str, error_msg: str) -> str: """Extract a required non-empty string field.""" value = raw.get(key, "") @@ -159,23 +368,71 @@ def _validate_repos(raw: dict[str, object]) -> list[str]: return repos +# --------------------------------------------------------------------------- +# Main validation +# --------------------------------------------------------------------------- + + def _validate(raw: dict[str, object]) -> Config: - """Validate raw TOML dict and return a Config instance.""" - github_token = _require_str(raw, "github_token", "github_token is required (set in config or GITHUB_TOKEN env var)") - github_username = _require_str(raw, "github_username", "github_username is required") + """Validate raw TOML dict and return a Config instance. + + Collects *all* validation errors and raises a single ``ConfigError`` + with every problem reported, so the user can fix everything in one pass. + """ + errors: list[str] = [] + + github_token = _collect_str( + raw, + "github_token", + "github_token is required (set in config.toml or export GITHUB_TOKEN=ghp_...)", + errors, + ) + github_username = _collect_str( + raw, + "github_username", + "github_username is required", + errors, + ) + + poll_interval = _collect_int_min(raw, "poll_interval", default=300, minimum=_MIN_POLL_INTERVAL, errors=errors) + # Enhance poll_interval error with recommendation if present. + for i, err in enumerate(errors): + if "poll_interval must be >=" in err: + errors[i] = f"{err} (GitHub recommends 300s for personal tokens)" + + repos = _collect_repos(raw, errors) + log_level = _collect_choice(raw, "log_level", default="info", choices=_VALID_LOG_LEVELS, errors=errors) + notify_on_first_poll = _collect_bool(raw, "notify_on_first_poll", default=False, errors=errors) + notifications_enabled = _collect_bool(raw, "notifications_enabled", default=True, errors=errors) + dbus_enabled = _collect_bool(raw, "dbus_enabled", default=True, errors=errors) + github_base_url = _collect_base_url(raw, errors) + max_retries = _collect_int_min(raw, "max_retries", default=3, minimum=0, errors=errors) + notification_threshold = _collect_int_min(raw, "notification_threshold", default=3, minimum=1, errors=errors) + notification_urgency = _collect_choice( + raw, + "notification_urgency", + default="normal", + choices=_VALID_URGENCIES, + errors=errors, + ) + icon_theme = _collect_choice(raw, "icon_theme", default="light", choices=_VALID_ICON_THEMES, errors=errors) + + if errors: + msg = "\n".join(errors) + raise ConfigError(msg) return Config( github_token=github_token, github_username=github_username, - poll_interval=_validate_int_min(raw, "poll_interval", default=300, minimum=_MIN_POLL_INTERVAL), - repos=_validate_repos(raw), - log_level=_validate_choice(raw, "log_level", default="info", choices=_VALID_LOG_LEVELS), - notify_on_first_poll=_validate_bool(raw, "notify_on_first_poll", default=False), - notifications_enabled=_validate_bool(raw, "notifications_enabled", default=True), - dbus_enabled=_validate_bool(raw, "dbus_enabled", default=True), - github_base_url=_validate_base_url(raw), - max_retries=_validate_int_min(raw, "max_retries", default=3, minimum=0), - notification_threshold=_validate_int_min(raw, "notification_threshold", default=3, minimum=1), - notification_urgency=_validate_choice(raw, "notification_urgency", default="normal", choices=_VALID_URGENCIES), - icon_theme=_validate_choice(raw, "icon_theme", default="light", choices=_VALID_ICON_THEMES), + poll_interval=poll_interval, + repos=repos, + log_level=log_level, + notify_on_first_poll=notify_on_first_poll, + notifications_enabled=notifications_enabled, + dbus_enabled=dbus_enabled, + github_base_url=github_base_url, + max_retries=max_retries, + notification_threshold=notification_threshold, + notification_urgency=notification_urgency, + icon_theme=icon_theme, ) diff --git a/forgewatch/indicator/__main__.py b/forgewatch/indicator/__main__.py index 743ce43..a0c8e09 100644 --- a/forgewatch/indicator/__main__.py +++ b/forgewatch/indicator/__main__.py @@ -100,15 +100,23 @@ def main() -> None: logger = logging.getLogger(__name__) - # Load icon_theme from config (best-effort: default to "light" on failure). + # Load config (best-effort: default to "light" theme and default + # IndicatorConfig on failure). icon_theme = "light" + indicator_config = None try: - from forgewatch.config import load_config # noqa: PLC0415 + from forgewatch.config import IndicatorConfig, load_config, load_indicator_config # noqa: PLC0415 cfg = load_config() icon_theme = cfg.icon_theme + indicator_config = load_indicator_config() except Exception: # noqa: BLE001 - logger.warning("Failed to load config, using default icon theme 'light'", exc_info=True) + logger.warning("Failed to load config, using defaults", exc_info=True) + + if indicator_config is None: + from forgewatch.config import IndicatorConfig # noqa: PLC0415 + + indicator_config = IndicatorConfig() # Install gbulb BEFORE obtaining an event loop so the GLib-based # policy is active from the start. @@ -120,7 +128,12 @@ def main() -> None: # which must only happen after dependency checks have passed. from .app import IndicatorApp # noqa: PLC0415 - app = IndicatorApp(icon_theme=icon_theme) + app = IndicatorApp( + icon_theme=icon_theme, + reconnect_interval=indicator_config.reconnect_interval, + window_width=indicator_config.window_width, + max_window_height=indicator_config.max_window_height, + ) loop = asyncio.new_event_loop() try: loop.run_until_complete(app.run()) diff --git a/forgewatch/indicator/app.py b/forgewatch/indicator/app.py index bdbddc1..603a7f8 100644 --- a/forgewatch/indicator/app.py +++ b/forgewatch/indicator/app.py @@ -38,7 +38,14 @@ class IndicatorApp: 3. ``shutdown()`` — disconnect from D-Bus, clean up resources. """ - def __init__(self, *, icon_theme: str = "light") -> None: + def __init__( + self, + *, + icon_theme: str = "light", + reconnect_interval: int = 10, + window_width: int = 400, + max_window_height: int = 500, + ) -> None: # Lazy imports — tray and window depend on GTK system packages # that are unavailable in headless CI environments. from .tray import TrayIcon # noqa: PLC0415 @@ -55,6 +62,7 @@ def __init__(self, *, icon_theme: str = "light") -> None: self._client = DaemonClient( on_prs_changed=self._on_prs_changed, on_connection_changed=self._on_connection_changed, + reconnect_interval=reconnect_interval, ) self._tray: TrayIcon = TrayIcon( on_activate=self._on_activate, @@ -66,6 +74,8 @@ def __init__(self, *, icon_theme: str = "light") -> None: on_pr_clicked=self._on_pr_clicked, on_refresh=self._on_refresh, on_visibility_changed=self._on_window_visibility_changed, + window_width=window_width, + max_window_height=max_window_height, ) # -- public lifecycle ---------------------------------------------------- diff --git a/forgewatch/indicator/client.py b/forgewatch/indicator/client.py index d5f4317..573840e 100644 --- a/forgewatch/indicator/client.py +++ b/forgewatch/indicator/client.py @@ -91,9 +91,12 @@ def __init__( self, on_prs_changed: Callable[[list[PRInfo]], None], on_connection_changed: Callable[[bool], None], + *, + reconnect_interval: int = _RECONNECT_INTERVAL_S, ) -> None: self._on_prs_changed = on_prs_changed self._on_connection_changed = on_connection_changed + self._reconnect_interval = reconnect_interval self._bus: MessageBus | None = None self._interface: ProxyInterface | None = None @@ -257,8 +260,8 @@ def _fire() -> None: self._reconnect_handle = None asyncio.ensure_future(self.connect()) # noqa: RUF006 - self._reconnect_handle = loop.call_later(_RECONNECT_INTERVAL_S, _fire) - logger.debug("Reconnect scheduled in %ds", _RECONNECT_INTERVAL_S) + self._reconnect_handle = loop.call_later(self._reconnect_interval, _fire) + logger.debug("Reconnect scheduled in %ds", self._reconnect_interval) def _cancel_reconnect(self) -> None: """Cancel a pending reconnection attempt, if any.""" diff --git a/forgewatch/indicator/window.py b/forgewatch/indicator/window.py index 09ddd3d..06a292f 100644 --- a/forgewatch/indicator/window.py +++ b/forgewatch/indicator/window.py @@ -116,10 +116,15 @@ def __init__( on_pr_clicked: Callable[[str], None], on_refresh: Callable[[], None], on_visibility_changed: Callable[[bool], None] | None = None, + *, + window_width: int = _WINDOW_WIDTH, + max_window_height: int = _MAX_WINDOW_HEIGHT, ) -> None: self._on_pr_clicked = on_pr_clicked self._on_refresh = on_refresh self._on_visibility_changed = on_visibility_changed + self._window_width = window_width + self._max_window_height = max_window_height # Maps ListBoxRow index → PR URL for click handling. self._row_urls: dict[int, str] = {} @@ -138,7 +143,7 @@ def __init__( # Content area that can be swapped between list and empty/disconnected state. self._scrolled = Gtk.ScrolledWindow() self._scrolled.set_policy(Gtk.PolicyType.NEVER, Gtk.PolicyType.AUTOMATIC) - self._scrolled.set_max_content_height(_MAX_WINDOW_HEIGHT - 100) + self._scrolled.set_max_content_height(self._max_window_height - 100) self._scrolled.set_propagate_natural_height(True) self._scrolled.add(self._listbox) @@ -233,7 +238,7 @@ def _build_window(self) -> Gtk.Window: window.set_skip_taskbar_hint(True) window.set_skip_pager_hint(True) window.set_type_hint(Gdk.WindowTypeHint.POPUP_MENU) - window.set_default_size(_WINDOW_WIDTH, -1) + window.set_default_size(self._window_width, -1) window.get_style_context().add_class("pr-window") window.connect("focus-out-event", self._on_focus_out) diff --git a/tests/test_config.py b/tests/test_config.py index 9b0e322..e8819fe 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -2,11 +2,12 @@ from __future__ import annotations +import logging from typing import TYPE_CHECKING import pytest -from forgewatch.config import Config, ConfigError, load_config +from forgewatch.config import Config, ConfigError, IndicatorConfig, load_config, load_indicator_config if TYPE_CHECKING: from pathlib import Path @@ -379,3 +380,243 @@ def test_poll_interval_at_boundary(tmp_path: Path) -> None: p.write_text('github_token = "ghp_abc"\ngithub_username = "user"\npoll_interval = 30\n') cfg = load_config(p) assert cfg.poll_interval == 30 + + +# --------------------------------------------------------------------------- +# Unknown keys warning (Step 3) +# --------------------------------------------------------------------------- + + +def test_unknown_key_logs_warning(tmp_path: Path, caplog: pytest.LogCaptureFixture) -> None: + """Unknown top-level config keys produce log warnings.""" + content = """\ +github_token = "ghp_abc" +github_username = "user" +typo_key = "oops" +another_unknown = 42 +""" + p = tmp_path / "config.toml" + p.write_text(content) + with caplog.at_level(logging.WARNING, logger="forgewatch.config"): + load_config(p) + assert any("'another_unknown'" in r.message for r in caplog.records) + assert any("'typo_key'" in r.message for r in caplog.records) + + +def test_known_keys_no_warning(tmp_path: Path, caplog: pytest.LogCaptureFixture) -> None: + """Known config keys should not produce warnings.""" + p = tmp_path / "config.toml" + p.write_text(MINIMAL_TOML) + with caplog.at_level(logging.WARNING, logger="forgewatch.config"): + load_config(p) + assert not any("Unknown config key" in r.message for r in caplog.records) + + +def test_indicator_section_is_known_key(tmp_path: Path, caplog: pytest.LogCaptureFixture) -> None: + """The [indicator] section key should be recognised as known.""" + content = """\ +github_token = "ghp_abc" +github_username = "user" + +[indicator] +reconnect_interval = 5 +""" + p = tmp_path / "config.toml" + p.write_text(content) + with caplog.at_level(logging.WARNING, logger="forgewatch.config"): + load_config(p) + assert not any("Unknown config key" in r.message for r in caplog.records) + + +# --------------------------------------------------------------------------- +# Multi-error collection (Step 3) +# --------------------------------------------------------------------------- + + +def test_multiple_errors_collected(tmp_path: Path) -> None: + """All validation errors are collected and reported in a single ConfigError.""" + content = """\ +poll_interval = 5 +log_level = "trace" +""" + p = tmp_path / "config.toml" + p.write_text(content) + with pytest.raises(ConfigError) as exc_info: + load_config(p) + msg = str(exc_info.value) + # Should contain errors for: missing token, missing username, poll_interval, log_level + assert "github_token is required" in msg + assert "github_username is required" in msg + assert "poll_interval must be >= 30" in msg + assert "log_level must be one of" in msg + + +def test_poll_interval_error_includes_recommendation(tmp_path: Path) -> None: + """poll_interval error should include the GitHub recommendation.""" + content = """\ +github_token = "ghp_abc" +github_username = "user" +poll_interval = 10 +""" + p = tmp_path / "config.toml" + p.write_text(content) + with pytest.raises(ConfigError, match="GitHub recommends 300s"): + load_config(p) + + +def test_repo_format_error_includes_example(tmp_path: Path) -> None: + """Invalid repo format error should include an example.""" + content = """\ +github_token = "ghp_abc" +github_username = "user" +repos = ["bad-repo-format"] +""" + p = tmp_path / "config.toml" + p.write_text(content) + with pytest.raises(ConfigError, match="octocat/Hello-World"): + load_config(p) + + +def test_token_error_includes_example_prefix(tmp_path: Path) -> None: + """Missing token error should include the ghp_ prefix example.""" + p = tmp_path / "config.toml" + p.write_text('github_username = "user"\n') + with pytest.raises(ConfigError, match="ghp_"): + load_config(p) + + +# --------------------------------------------------------------------------- +# IndicatorConfig — load_indicator_config (Step 4) +# --------------------------------------------------------------------------- + + +def test_load_indicator_config_defaults(tmp_path: Path) -> None: + """Missing [indicator] section returns all defaults.""" + p = tmp_path / "config.toml" + p.write_text(MINIMAL_TOML) + cfg = load_indicator_config(p) + assert cfg == IndicatorConfig() + assert cfg.reconnect_interval == 10 + assert cfg.window_width == 400 + assert cfg.max_window_height == 500 + + +def test_load_indicator_config_with_values(tmp_path: Path) -> None: + """Custom [indicator] values are loaded correctly.""" + content = """\ +github_token = "ghp_abc" +github_username = "user" + +[indicator] +reconnect_interval = 30 +window_width = 600 +max_window_height = 800 +""" + p = tmp_path / "config.toml" + p.write_text(content) + cfg = load_indicator_config(p) + assert cfg.reconnect_interval == 30 + assert cfg.window_width == 600 + assert cfg.max_window_height == 800 + + +def test_load_indicator_config_partial_values(tmp_path: Path) -> None: + """Partial [indicator] section uses defaults for missing keys.""" + content = """\ +github_token = "ghp_abc" +github_username = "user" + +[indicator] +reconnect_interval = 5 +""" + p = tmp_path / "config.toml" + p.write_text(content) + cfg = load_indicator_config(p) + assert cfg.reconnect_interval == 5 + assert cfg.window_width == 400 # default + assert cfg.max_window_height == 500 # default + + +def test_load_indicator_config_missing_file(tmp_path: Path) -> None: + """Missing config file returns defaults (no error).""" + cfg = load_indicator_config(tmp_path / "nonexistent.toml") + assert cfg == IndicatorConfig() + + +def test_load_indicator_config_invalid_toml(tmp_path: Path) -> None: + """Invalid TOML returns defaults (no error).""" + p = tmp_path / "config.toml" + p.write_text("this is [not valid toml =") + cfg = load_indicator_config(p) + assert cfg == IndicatorConfig() + + +def test_load_indicator_config_reconnect_interval_too_low(tmp_path: Path) -> None: + """reconnect_interval < 1 raises ConfigError.""" + content = """\ +[indicator] +reconnect_interval = 0 +""" + p = tmp_path / "config.toml" + p.write_text(content) + with pytest.raises(ConfigError, match="reconnect_interval must be >= 1"): + load_indicator_config(p) + + +def test_load_indicator_config_window_width_too_low(tmp_path: Path) -> None: + """window_width < 200 raises ConfigError.""" + content = """\ +[indicator] +window_width = 100 +""" + p = tmp_path / "config.toml" + p.write_text(content) + with pytest.raises(ConfigError, match="window_width must be >= 200"): + load_indicator_config(p) + + +def test_load_indicator_config_max_window_height_too_low(tmp_path: Path) -> None: + """max_window_height < 200 raises ConfigError.""" + content = """\ +[indicator] +max_window_height = 50 +""" + p = tmp_path / "config.toml" + p.write_text(content) + with pytest.raises(ConfigError, match="max_window_height must be >= 200"): + load_indicator_config(p) + + +def test_load_indicator_config_wrong_type(tmp_path: Path) -> None: + """Non-integer values raise ConfigError.""" + content = """\ +[indicator] +reconnect_interval = "fast" +""" + p = tmp_path / "config.toml" + p.write_text(content) + with pytest.raises(ConfigError, match="reconnect_interval must be an integer"): + load_indicator_config(p) + + +def test_load_indicator_config_multiple_errors(tmp_path: Path) -> None: + """Multiple indicator validation errors are collected.""" + content = """\ +[indicator] +reconnect_interval = 0 +window_width = 50 +""" + p = tmp_path / "config.toml" + p.write_text(content) + with pytest.raises(ConfigError) as exc_info: + load_indicator_config(p) + msg = str(exc_info.value) + assert "reconnect_interval" in msg + assert "window_width" in msg + + +def test_indicator_config_frozen() -> None: + """IndicatorConfig is a frozen dataclass.""" + cfg = IndicatorConfig() + with pytest.raises(AttributeError): + cfg.reconnect_interval = 99 # type: ignore[misc] diff --git a/tests/test_indicator_app.py b/tests/test_indicator_app.py index 3306ada..9852cd3 100644 --- a/tests/test_indicator_app.py +++ b/tests/test_indicator_app.py @@ -118,11 +118,19 @@ def _create_app( mock_window_cls: MagicMock, *, icon_theme: str = "light", + reconnect_interval: int = 10, + window_width: int = 400, + max_window_height: int = 500, ) -> tuple[Any, MagicMock, MagicMock, MagicMock]: """Import and create IndicatorApp with all GTK components mocked.""" from forgewatch.indicator.app import IndicatorApp - app = IndicatorApp(icon_theme=icon_theme) + app = IndicatorApp( + icon_theme=icon_theme, + reconnect_interval=reconnect_interval, + window_width=window_width, + max_window_height=max_window_height, + ) # Return convenience references to the mock instances. client = mock_client_cls.return_value tray = mock_tray_cls.return_value @@ -175,6 +183,21 @@ def test_creates_window_with_callbacks( assert "on_refresh" in kwargs.kwargs assert "on_visibility_changed" in kwargs.kwargs + def test_passes_reconnect_interval_to_client( + self, mock_client_cls: MagicMock, mock_tray_cls: MagicMock, mock_window_cls: MagicMock + ) -> None: + _create_app(mock_client_cls, mock_tray_cls, mock_window_cls, reconnect_interval=42) + kwargs = mock_client_cls.call_args + assert kwargs.kwargs["reconnect_interval"] == 42 + + def test_passes_window_dimensions_to_window( + self, mock_client_cls: MagicMock, mock_tray_cls: MagicMock, mock_window_cls: MagicMock + ) -> None: + _create_app(mock_client_cls, mock_tray_cls, mock_window_cls, window_width=600, max_window_height=800) + kwargs = mock_window_cls.call_args + assert kwargs.kwargs["window_width"] == 600 + assert kwargs.kwargs["max_window_height"] == 800 + # --------------------------------------------------------------------------- # Tests: run() lifecycle diff --git a/tests/test_indicator_client.py b/tests/test_indicator_client.py index f37425c..a41b658 100644 --- a/tests/test_indicator_client.py +++ b/tests/test_indicator_client.py @@ -713,6 +713,22 @@ async def test_reconnect_scheduled_on_failure(self, mock_bus_class: MagicMock) - assert client._reconnect_handle.cancelled() is False client._cancel_reconnect() + @patch("forgewatch.indicator.client.MessageBus") + async def test_custom_reconnect_interval(self, mock_bus_class: MagicMock) -> None: + """Custom reconnect_interval is stored and used by _schedule_reconnect.""" + mock_bus_class.return_value.connect = AsyncMock(side_effect=OSError("no bus")) + + client = DaemonClient( + on_prs_changed=MagicMock(), + on_connection_changed=MagicMock(), + reconnect_interval=42, + ) + assert client._reconnect_interval == 42 + + await client.connect() + assert client._reconnect_handle is not None + client._cancel_reconnect() + @patch("forgewatch.indicator.client.MessageBus") async def test_reconnect_not_doubled(self, mock_bus_class: MagicMock) -> None: """Multiple failures should not schedule multiple reconnects.""" diff --git a/tests/test_indicator_main.py b/tests/test_indicator_main.py index 6f3c697..18f1d62 100644 --- a/tests/test_indicator_main.py +++ b/tests/test_indicator_main.py @@ -328,6 +328,11 @@ def test_uses_default_icon_theme(self) -> None: mock_loop = MagicMock() mock_config_mod = MagicMock() mock_config_mod.load_config.side_effect = Exception("no config file") + mock_config_mod.IndicatorConfig.return_value = MagicMock( + reconnect_interval=10, + window_width=400, + max_window_height=500, + ) mock_app_mod = MagicMock() mock_app_mod.IndicatorApp = mock_app_class @@ -347,4 +352,10 @@ def test_uses_default_icon_theme(self) -> None: mod.main() # IndicatorApp should be constructed with icon_theme="light" (the default) - mock_app_class.assert_called_once_with(icon_theme="light") + # and default indicator config values. + mock_app_class.assert_called_once() + call_kwargs = mock_app_class.call_args.kwargs + assert call_kwargs["icon_theme"] == "light" + assert call_kwargs["reconnect_interval"] == 10 + assert call_kwargs["window_width"] == 400 + assert call_kwargs["max_window_height"] == 500 diff --git a/tests/test_indicator_window.py b/tests/test_indicator_window.py index 4bd4705..089d0ac 100644 --- a/tests/test_indicator_window.py +++ b/tests/test_indicator_window.py @@ -360,6 +360,24 @@ def test_visibility_changed_callback_optional(self) -> None: assert win._on_visibility_changed is None + def test_default_dimensions(self) -> None: + """Default window dimensions match module constants.""" + win = PRWindow(MagicMock(), MagicMock()) + + assert win._window_width == 400 + assert win._max_window_height == 500 + + def test_custom_dimensions(self) -> None: + """Custom dimensions are stored and used.""" + win = PRWindow(MagicMock(), MagicMock(), window_width=600, max_window_height=800) + + assert win._window_width == 600 + assert win._max_window_height == 800 + # Verify set_default_size was called with custom width + win._window.set_default_size.assert_called_with(600, -1) + # Verify set_max_content_height was called with custom height - 100 + win._scrolled.set_max_content_height.assert_called_with(700) + # --------------------------------------------------------------------------- # PRWindow — update_prs From d2eca3280a46e88b29866b79a7ae27fa1494e7c4 Mon Sep 17 00:00:00 2001 From: Jan Dvorak Date: Fri, 13 Mar 2026 11:58:33 +0100 Subject: [PATCH 3/4] add repo-based notification grouping with per-repo config --- CHANGELOG.md | 2 + config.example.toml | 8 + docs/configuration.md | 56 +++++++ docs/modules/config.md | 68 ++++++++ docs/modules/daemon.md | 6 +- docs/modules/notifier.md | 120 ++++++++++++-- forgewatch/config.py | 81 ++++++++++ forgewatch/daemon.py | 2 + forgewatch/notifier.py | 114 ++++++++++++- tests/test_config.py | 157 ++++++++++++++++++ tests/test_daemon.py | 75 +++++++++ tests/test_notifier.py | 341 +++++++++++++++++++++++++++++++++++++++ 12 files changed, 1015 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cae70b3..c1fb814 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Pagination cap warning -- the poller now logs a warning when the page limit is reached and more results are available, suggesting the user narrow their repo filter - Configurable indicator settings via a new `[indicator]` TOML section -- `reconnect_interval`, `window_width`, and `max_window_height` can now be tuned without code changes - Unknown config key warnings -- `load_config()` now logs a warning for any unrecognised top-level key, helping catch typos early +- Repo-based notification grouping via a new `[notifications]` config section -- set `grouping = "repo"` to receive per-repository summary notifications instead of a single flat list +- Per-repo notification overrides via `[notifications.repos."owner/repo"]` -- disable notifications for noisy repos (`enabled = false`), override urgency (`urgency = "critical"`), or set a custom individual-vs-summary threshold per repository ### Changed diff --git a/config.example.toml b/config.example.toml index 83cbb11..2d66f34 100644 --- a/config.example.toml +++ b/config.example.toml @@ -49,6 +49,14 @@ repos = [] # Use "dark" for dark desktop panels (light icons on dark background). # icon_theme = "light" +# [notifications] +# grouping = "flat" # "flat" (default) or "repo" +# +# [notifications.repos."owner/repo"] +# enabled = true # Set to false to suppress notifications +# urgency = "normal" # "low", "normal", or "critical" +# threshold = 3 # Individual vs summary threshold + # --------------------------------------------------------------------------- # Indicator settings (system tray process) # --------------------------------------------------------------------------- diff --git a/docs/configuration.md b/docs/configuration.md index b578c68..b862fe1 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -38,6 +38,26 @@ If no config file is found at the resolved path, a `ConfigError` is raised. | `notification_urgency` | string | No | `"normal"` | Notification urgency: `"low"`, `"normal"`, or `"critical"` | | `icon_theme` | string | No | `"light"` | Icon theme for the system tray indicator: `"light"` (dark icons for light panels) or `"dark"` (light icons for dark panels) | +### `[notifications]` section + +Settings for notification grouping and per-repo overrides. These affect how +desktop notifications are grouped and allow fine-grained control per repository. + +| Field | Type | Required | Default | Description | +|---|---|---|---|---| +| `grouping` | string | No | `"flat"` | Grouping mode: `"flat"` (single list) or `"repo"` (grouped by repository) | + +#### `[notifications.repos."owner/repo"]` sub-tables + +Per-repo notification overrides. Each key is a repository in `owner/name` +format. Repos without an entry use the global defaults. + +| Field | Type | Required | Default | Description | +|---|---|---|---|---| +| `enabled` | boolean | No | `true` | Set to `false` to suppress notifications for this repo | +| `urgency` | string | No | `"normal"` | Notification urgency: `"low"`, `"normal"`, or `"critical"` | +| `threshold` | integer | No | `3` | Individual vs. summary threshold for this repo (minimum: 1) | + ### `[indicator]` section Settings for the system tray indicator process. These are read by the @@ -161,6 +181,16 @@ Unrecognised top-level keys produce a log warning (possible typo detection). - `notification_urgency` -- must be one of `low`, `normal`, `critical` (case-insensitive) - `icon_theme` -- must be one of `light`, `dark` (case-insensitive) +**`[notifications]` section:** + +- `notifications` -- must be a table (if present) +- `notifications.grouping` -- must be one of `flat`, `repo` (case-insensitive) +- `notifications.repos` -- must be a table (if present) +- Each `notifications.repos."owner/repo"` entry must be a table with: + - `enabled` -- must be a boolean + - `urgency` -- must be one of `low`, `normal`, `critical` (case-insensitive) + - `threshold` -- must be an integer >= 1 + **`[indicator]` section:** - `reconnect_interval` -- must be an integer >= 1 @@ -196,6 +226,16 @@ notification_threshold = 5 notification_urgency = "low" icon_theme = "light" +[notifications] +grouping = "repo" + +[notifications.repos."myorg/frontend"] +urgency = "critical" +threshold = 5 + +[notifications.repos."myorg/noisy-repo"] +enabled = false + [indicator] reconnect_interval = 10 window_width = 400 @@ -264,6 +304,18 @@ repos = [] # Use "dark" for dark desktop panels (light icons on dark background). # icon_theme = "light" +# --------------------------------------------------------------------------- +# Notification grouping and per-repo overrides +# --------------------------------------------------------------------------- + +# [notifications] +# grouping = "flat" # "flat" (default) or "repo" +# +# [notifications.repos."owner/repo"] +# enabled = true # Set to false to suppress notifications +# urgency = "normal" # "low", "normal", or "critical" +# threshold = 3 # Individual vs summary threshold + # --------------------------------------------------------------------------- # Indicator settings (system tray process) # --------------------------------------------------------------------------- @@ -318,6 +370,10 @@ print(cfg.notification_threshold) # 3 print(cfg.notification_urgency) # "normal" print(cfg.icon_theme) # "light" +# Access notification grouping settings +print(cfg.notifications.grouping) # "flat" +print(cfg.notifications.repos) # {} (or dict of RepoNotificationConfig) + # Load indicator-specific config ([indicator] section) ind = load_indicator_config() print(ind.reconnect_interval) # 10 diff --git a/docs/modules/config.md b/docs/modules/config.md index f6015ea..3b5b70b 100644 --- a/docs/modules/config.md +++ b/docs/modules/config.md @@ -20,6 +20,7 @@ Internal constants (prefixed with `_`): | `_VALID_LOG_LEVELS` | `frozenset[str]` | `{"debug", "info", "warning", "error"}` | Allowed values for `log_level` | | `_VALID_URGENCIES` | `frozenset[str]` | `{"low", "normal", "critical"}` | Allowed values for `notification_urgency` | | `_VALID_ICON_THEMES` | `frozenset[str]` | `{"light", "dark"}` | Allowed values for `icon_theme` | +| `_VALID_GROUPING_MODES` | `frozenset[str]` | `{"flat", "repo"}` | Allowed values for `notifications.grouping` | | `_KNOWN_KEYS` | `frozenset[str]` | *(all recognised top-level keys)* | Used by `_warn_unknown_keys()` to detect typos | ## `ConfigError` @@ -52,6 +53,41 @@ every problem listed (one per line), so the user can fix everything in one pass. - `"notification_urgency must be one of ['critical', 'low', 'normal'], got 'extreme'"` - `"icon_theme must be one of ['dark', 'light'], got 'blue'"` +## `RepoNotificationConfig` + +```python +@dataclass(frozen=True) +class RepoNotificationConfig: + enabled: bool = True + urgency: str = "normal" + threshold: int = 3 +``` + +Per-repo notification overrides. Each instance corresponds to a +`[notifications.repos."owner/repo"]` entry in the config file. + +| Field | Type | Default | Description | +|---|---|---|---| +| `enabled` | `bool` | `True` | Set to `False` to suppress notifications for this repo | +| `urgency` | `str` | `"normal"` | Notification urgency: `low`, `normal`, `critical` | +| `threshold` | `int` | `3` | Individual vs. summary notification cutoff (>= 1) | + +## `NotificationConfig` + +```python +@dataclass(frozen=True) +class NotificationConfig: + grouping: str = "flat" + repos: dict[str, RepoNotificationConfig] = field(default_factory=dict) +``` + +Notification grouping and per-repo settings from the `[notifications]` section. + +| Field | Type | Default | Description | +|---|---|---|---| +| `grouping` | `str` | `"flat"` | Grouping mode: `flat` (single list) or `repo` (grouped by repository) | +| `repos` | `dict[str, RepoNotificationConfig]` | `{}` | Per-repo overrides keyed by `owner/name` | + ## `Config` ```python @@ -70,6 +106,7 @@ class Config: notification_threshold: int = 3 notification_urgency: str = "normal" icon_theme: str = "light" + notifications: NotificationConfig = field(default_factory=NotificationConfig) ``` An immutable (frozen) dataclass holding validated configuration values. @@ -89,6 +126,7 @@ An immutable (frozen) dataclass holding validated configuration values. | `notification_threshold` | `int` | `3` | Individual vs. summary notification cutoff (>= 1) | | `notification_urgency` | `str` | `"normal"` | Notification urgency: `low`, `normal`, `critical` | | `icon_theme` | `str` | `"light"` | Icon theme for tray indicator: `light`, `dark` | +| `notifications` | `NotificationConfig` | `NotificationConfig()` | Notification grouping and per-repo overrides | The dataclass is frozen, so fields cannot be modified after creation: @@ -329,6 +367,32 @@ one pass. 11. `notification_threshold` -- must be an `int` >= 1 (via `_collect_int_min`) 12. `notification_urgency` -- must be one of `_VALID_URGENCIES` (via `_collect_choice`) 13. `icon_theme` -- must be one of `_VALID_ICON_THEMES` (via `_collect_choice`) +14. `notifications` -- validated via `_validate_notifications()` (see below) + +## `_validate_notifications()` (internal) + +```python +def _validate_notifications(raw: dict[str, object]) -> NotificationConfig: +``` + +Validates the `[notifications]` TOML section. Returns `NotificationConfig()` +with defaults if the section is absent. Checks: + +- `notifications` must be a table (if present) +- `grouping` must be a string in `_VALID_GROUPING_MODES` (case-insensitive) +- `repos` must be a table (if present); each entry validated by `_validate_repo_notification()` + +## `_validate_repo_notification()` (internal) + +```python +def _validate_repo_notification(repo_name: str, repo_raw: dict[str, object]) -> RepoNotificationConfig: +``` + +Validates a single `[notifications.repos."owner/repo"]` entry. Checks: + +- `enabled` must be a boolean (default: `True`) +- `urgency` must be a string in `_VALID_URGENCIES` (case-insensitive, default: `"normal"`) +- `threshold` must be an integer >= 1 (default: `3`) ## Tests @@ -350,3 +414,7 @@ Tests in `tests/test_config.py` covering: - `IndicatorConfig` via `load_indicator_config()` -- defaults, custom values, partial values, missing file, invalid TOML, boundary validation for each field, wrong types, multiple errors collected, frozen dataclass +- `NotificationConfig` -- defaults when section missing, grouping modes (flat, + repo), invalid grouping, case-insensitive grouping, per-repo full config, + disabled repos, invalid urgency/threshold/enabled types, multiple repos, + `notifications` not a table diff --git a/docs/modules/daemon.md b/docs/modules/daemon.md index 3810923..5aeeba8 100644 --- a/docs/modules/daemon.md +++ b/docs/modules/daemon.md @@ -84,6 +84,10 @@ Single poll cycle: `threshold` parameter (individual vs. summary notification cutoff) - `config.notification_urgency` -- passed to `notify_new_prs()` as the `urgency` parameter +- `config.notifications.grouping` -- passed to `notify_new_prs()` as the + `grouping` parameter (`"flat"` or `"repo"`) +- `config.notifications.repos` -- passed to `notify_new_prs()` as the + `repo_overrides` parameter (`None` if the dict is empty) **First-poll notification suppression:** On the very first poll cycle, all PRs appear as "new" because the store starts empty. By default @@ -135,7 +139,7 @@ Daemon._poll_once() ├── store.update(prs) ──► StateDiff │ ├── if new PRs AND notifications_enabled AND (not first_poll OR notify_on_first_poll): - │ └── notify_new_prs(threshold=..., urgency=...) ──► notify-send + │ └── notify_new_prs(threshold=..., urgency=..., grouping=..., repo_overrides=...) ──► notify-send │ └── if any changes AND dbus connected: └── interface.PullRequestsChanged() ──► D-Bus signal diff --git a/docs/modules/notifier.md b/docs/modules/notifier.md index bb92727..f1e1eb0 100644 --- a/docs/modules/notifier.md +++ b/docs/modules/notifier.md @@ -21,6 +21,7 @@ Module-level state: |---|---|---| | `_avatar_cache` | `dict[str, Path]` | In-memory cache mapping avatar URLs to local file paths | | `_background_tasks` | `set[asyncio.Task]` | Tracks background tasks (notification click handlers) to prevent GC | +| `_DEFAULT_REPO_OVERRIDE` | `RepoNotificationConfig` | Default repo override instance (all defaults) used when a repo has no explicit config | ## Functions @@ -32,6 +33,8 @@ async def notify_new_prs( *, threshold: int = _INDIVIDUAL_THRESHOLD, urgency: str = "normal", + grouping: str = "flat", + repo_overrides: dict[str, RepoNotificationConfig] | None = None, ) -> None: ``` @@ -45,19 +48,32 @@ entry point -- call it after each poll cycle with the `StateDiff.new_prs` list. | `new_prs` | `list[PullRequest]` | (required) | PRs not seen in the previous poll cycle | | `threshold` | `int` | `3` | Max PRs for individual notifications; above this, a summary is sent | | `urgency` | `str` | `"normal"` | Notification urgency level: `low`, `normal`, or `critical` | +| `grouping` | `str` | `"flat"` | Grouping mode: `"flat"` (single list) or `"repo"` (grouped by repository) | +| `repo_overrides` | `dict[str, RepoNotificationConfig] \| None` | `None` | Per-repo settings: disable notifications, override urgency or threshold | **Behaviour:** - If `new_prs` is empty, returns immediately (no subprocess calls). -- If 1-`threshold` new PRs: opens a shared `aiohttp.ClientSession` and sends - one notification per PR with: - - Summary: `"PR Review: {repo_full_name}"` - - Body: `"#{number} {title}\nby {author}"` - - Icon: author's avatar (downloaded and cached locally) - - Action: clickable "Open" button that opens the PR in the default browser -- If > `threshold` new PRs: sends a single batch notification with: - - Summary: `"{count} new PR review requests"` - - Body: first 5 PRs as `"- {repo}#{number}: {title}"`, one per line +- PRs from repos with `enabled = false` in `repo_overrides` are filtered out + before any notifications are sent. +- **Flat mode** (`grouping="flat"`, the default): + - If 1-`threshold` new PRs: opens a shared `aiohttp.ClientSession` and sends + one notification per PR with per-repo urgency override (if configured): + - Summary: `"PR Review: {repo_full_name}"` + - Body: `"#{number} {title}\nby {author}"` + - Icon: author's avatar (downloaded and cached locally) + - Action: clickable "Open" button that opens the PR in the default browser + - If > `threshold` new PRs: sends a single batch notification with: + - Summary: `"{count} new PR review requests"` + - Body: first 5 PRs as `"- {repo}#{number}: {title}"`, one per line +- **Repo mode** (`grouping="repo"`): + - PRs are sorted and grouped by `repo_full_name`. + - Each repo group is handled independently using that repo's threshold + (from `repo_overrides`, falling back to the global `threshold`). + - Groups at or below the repo threshold get individual notifications. + - Groups above the repo threshold get a single repo-level summary: + - Summary: `"{count} new PRs in {repo_name}"` + - Body: first 5 PRs as `"- #{number}: {title}"`, one per line ### `_send_notification()` @@ -147,10 +163,81 @@ Desktop Portal first, then falls back to `xdg-open`). See [url_opener.md](url_opener.md) for details on the URL opening mechanism. +### `_notify_flat()` (internal) + +```python +async def _notify_flat( + new_prs: list[PullRequest], + *, + threshold: int, + urgency: str, + repo_overrides: dict[str, RepoNotificationConfig] | None, +) -> None: +``` + +Sends notifications in flat mode (original behaviour). Filters disabled repos, +then applies the global threshold to decide between individual and summary +notifications. Per-repo urgency overrides are applied to individual notifications. + +### `_notify_grouped_by_repo()` (internal) + +```python +async def _notify_grouped_by_repo( + new_prs: list[PullRequest], + *, + threshold: int, + urgency: str, + repo_overrides: dict[str, RepoNotificationConfig] | None, +) -> None: +``` + +Sends notifications grouped by repository. Filters disabled repos, sorts PRs by +`repo_full_name`, and processes each repo group independently using per-repo +threshold and urgency settings. + +### `_filter_disabled_repos()` (internal) + +```python +def _filter_disabled_repos( + prs: list[PullRequest], + repo_overrides: dict[str, RepoNotificationConfig] | None, +) -> list[PullRequest]: +``` + +Removes PRs from repos that have `enabled = False` in `repo_overrides`. Returns +all PRs unchanged if `repo_overrides` is `None` or empty. + +### `_get_repo_urgency()` (internal) + +```python +def _get_repo_urgency( + repo_name: str, + default_urgency: str, + repo_overrides: dict[str, RepoNotificationConfig] | None, +) -> str: +``` + +Returns the urgency for a given repo, falling back to the global default if the +repo has no override. + +### `_get_repo_threshold()` (internal) + +```python +def _get_repo_threshold( + repo_name: str, + default_threshold: int, + repo_overrides: dict[str, RepoNotificationConfig] | None, +) -> int: +``` + +Returns the threshold for a given repo, falling back to the global default if +the repo has no override. + ## Usage example ```python from forgewatch.notifier import notify_new_prs +from forgewatch.config import RepoNotificationConfig from forgewatch.store import PRStore store = PRStore() @@ -162,6 +249,16 @@ if diff.new_prs: # With custom threshold and urgency: await notify_new_prs(diff.new_prs, threshold=5, urgency="critical") + +# With repo grouping: +await notify_new_prs(diff.new_prs, grouping="repo") + +# With per-repo overrides: +overrides = { + "acme/web": RepoNotificationConfig(urgency="critical", threshold=5), + "acme/noisy": RepoNotificationConfig(enabled=False), +} +await notify_new_prs(diff.new_prs, grouping="repo", repo_overrides=overrides) ``` ## Design notes @@ -183,6 +280,11 @@ await notify_new_prs(diff.new_prs, threshold=5, urgency="critical") - The individual vs. batch threshold prevents desktop notification floods when many PRs appear at once (e.g., first poll after starting the daemon). The threshold is configurable via the `threshold` parameter +- Repo grouping mode (`grouping="repo"`) uses `itertools.groupby` after sorting + by `repo_full_name`, so each repo's PRs are handled independently with their + own threshold/urgency settings +- Per-repo overrides allow disabling notifications for noisy repos, setting + repo-specific urgency levels, or adjusting the summary threshold per repo - Background tasks are stored in `_background_tasks` to prevent garbage collection before completion - All errors are caught and logged -- the notifier never raises exceptions diff --git a/forgewatch/config.py b/forgewatch/config.py index d8c4313..fe1e01f 100644 --- a/forgewatch/config.py +++ b/forgewatch/config.py @@ -18,6 +18,7 @@ _VALID_LOG_LEVELS = frozenset({"debug", "info", "warning", "error"}) _VALID_URGENCIES = frozenset({"low", "normal", "critical"}) _VALID_ICON_THEMES = frozenset({"light", "dark"}) +_VALID_GROUPING_MODES = frozenset({"flat", "repo"}) _KNOWN_KEYS = frozenset( { @@ -46,6 +47,23 @@ class ConfigError(Exception): """Raised when configuration is invalid or cannot be loaded.""" +@dataclass(frozen=True) +class RepoNotificationConfig: + """Per-repo notification overrides.""" + + enabled: bool = True + urgency: str = "normal" + threshold: int = 3 + + +@dataclass(frozen=True) +class NotificationConfig: + """Notification grouping and per-repo settings.""" + + grouping: str = "flat" + repos: dict[str, RepoNotificationConfig] = field(default_factory=dict) + + @dataclass(frozen=True) class Config: """Validated configuration for forgewatch.""" @@ -63,6 +81,7 @@ class Config: notification_threshold: int = 3 notification_urgency: str = "normal" icon_theme: str = "light" + notifications: NotificationConfig = field(default_factory=NotificationConfig) @dataclass(frozen=True) @@ -368,6 +387,67 @@ def _validate_repos(raw: dict[str, object]) -> list[str]: return repos +def _validate_repo_notification(repo_name: str, repo_raw: dict[str, object]) -> RepoNotificationConfig: + """Validate a single ``[notifications.repos."owner/repo"]`` entry.""" + enabled = repo_raw.get("enabled", True) + if not isinstance(enabled, bool): + msg = f"notifications.repos.{repo_name!r}.enabled must be a boolean, got {type(enabled).__name__}" + raise ConfigError(msg) + + urgency = repo_raw.get("urgency", "normal") + if not isinstance(urgency, str): + msg = f"notifications.repos.{repo_name!r}.urgency must be a string, got {type(urgency).__name__}" + raise ConfigError(msg) + urgency = urgency.lower() + if urgency not in _VALID_URGENCIES: + msg = f"notifications.repos.{repo_name!r}.urgency must be one of {sorted(_VALID_URGENCIES)}, got {urgency!r}" + raise ConfigError(msg) + + threshold = repo_raw.get("threshold", 3) + if not isinstance(threshold, int): + msg = f"notifications.repos.{repo_name!r}.threshold must be an integer, got {type(threshold).__name__}" + raise ConfigError(msg) + if threshold < 1: + msg = f"notifications.repos.{repo_name!r}.threshold must be >= 1, got {threshold}" + raise ConfigError(msg) + + return RepoNotificationConfig(enabled=enabled, urgency=urgency, threshold=threshold) + + +def _validate_notifications(raw: dict[str, object]) -> NotificationConfig: + """Validate the ``[notifications]`` section and return a NotificationConfig.""" + section = raw.get("notifications") + if section is None: + return NotificationConfig() + + if not isinstance(section, dict): + msg = "notifications must be a table" + raise ConfigError(msg) + + raw_grouping = section.get("grouping", "flat") + if not isinstance(raw_grouping, str): + msg = f"notifications.grouping must be a string, got {type(raw_grouping).__name__}" + raise ConfigError(msg) + grouping = raw_grouping.lower() + if grouping not in _VALID_GROUPING_MODES: + msg = f"notifications.grouping must be one of {sorted(_VALID_GROUPING_MODES)}, got {grouping!r}" + raise ConfigError(msg) + + repos_raw = section.get("repos", {}) + if not isinstance(repos_raw, dict): + msg = "notifications.repos must be a table" + raise ConfigError(msg) + + repo_configs: dict[str, RepoNotificationConfig] = {} + for repo_name, repo_raw in repos_raw.items(): + if not isinstance(repo_raw, dict): + msg = f"notifications.repos.{repo_name!r} must be a table" + raise ConfigError(msg) + repo_configs[repo_name] = _validate_repo_notification(repo_name, repo_raw) + + return NotificationConfig(grouping=grouping, repos=repo_configs) + + # --------------------------------------------------------------------------- # Main validation # --------------------------------------------------------------------------- @@ -435,4 +515,5 @@ def _validate(raw: dict[str, object]) -> Config: notification_threshold=notification_threshold, notification_urgency=notification_urgency, icon_theme=icon_theme, + notifications=_validate_notifications(raw), ) diff --git a/forgewatch/daemon.py b/forgewatch/daemon.py index f74d9d7..ff58d70 100644 --- a/forgewatch/daemon.py +++ b/forgewatch/daemon.py @@ -145,6 +145,8 @@ async def _poll_once(self) -> None: diff.new_prs, threshold=self.config.notification_threshold, urgency=self.config.notification_urgency, + grouping=self.config.notifications.grouping, + repo_overrides=self.config.notifications.repos or None, ) if diff.has_changes and self.interface is not None: diff --git a/forgewatch/notifier.py b/forgewatch/notifier.py index 1185017..6c886be 100644 --- a/forgewatch/notifier.py +++ b/forgewatch/notifier.py @@ -12,6 +12,7 @@ import asyncio import hashlib +import itertools import logging import os from pathlib import Path @@ -24,6 +25,8 @@ if TYPE_CHECKING: from .poller import PullRequest +from .config import RepoNotificationConfig + logger = logging.getLogger(__name__) # Maximum number of PRs to show as individual notifications. @@ -49,6 +52,9 @@ # garbage-collected before completion. _background_tasks: set[asyncio.Task[None]] = set() +# Default repo override — used when a repo has no explicit config. +_DEFAULT_REPO_OVERRIDE = RepoNotificationConfig() + async def _download_avatar(avatar_url: str, session: aiohttp.ClientSession) -> str | None: """Download a GitHub avatar to a local temp file. @@ -113,6 +119,8 @@ async def notify_new_prs( *, threshold: int = _INDIVIDUAL_THRESHOLD, urgency: str = "normal", + grouping: str = "flat", + repo_overrides: dict[str, RepoNotificationConfig] | None = None, ) -> None: """Send desktop notifications for newly discovered PRs. @@ -122,30 +130,126 @@ async def notify_new_prs( If there are more than *threshold*, a single summary notification is sent listing up to the first 5 PRs to avoid desktop spam. + + When *grouping* is ``"repo"``, PRs are grouped by repository and + each group is handled independently (its own threshold / summary). + + *repo_overrides* allows per-repo settings: disable notifications + for specific repos, or override urgency / threshold. """ if not new_prs: return - if len(new_prs) <= threshold: + if grouping == "repo": + await _notify_grouped_by_repo(new_prs, threshold=threshold, urgency=urgency, repo_overrides=repo_overrides) + else: + await _notify_flat(new_prs, threshold=threshold, urgency=urgency, repo_overrides=repo_overrides) + + +async def _notify_flat( + new_prs: list[PullRequest], + *, + threshold: int, + urgency: str, + repo_overrides: dict[str, RepoNotificationConfig] | None, +) -> None: + """Send notifications in flat mode (original behaviour).""" + filtered = _filter_disabled_repos(new_prs, repo_overrides) + if not filtered: + return + + if len(filtered) <= threshold: async with aiohttp.ClientSession() as session: - for pr in new_prs: + for pr in filtered: + pr_urgency = _get_repo_urgency(pr.repo_full_name, urgency, repo_overrides) icon = await _download_avatar(pr.author_avatar_url, session) await _send_notification( summary=f"PR Review: {pr.repo_full_name}", body=f"#{pr.number} {pr.title}\nby {pr.author}", url=pr.url, icon=icon, - urgency=urgency, + urgency=pr_urgency, ) else: - body = "\n".join(f"- {pr.repo_full_name}#{pr.number}: {pr.title}" for pr in new_prs[:_BATCH_BODY_LIMIT]) + body = "\n".join(f"- {pr.repo_full_name}#{pr.number}: {pr.title}" for pr in filtered[:_BATCH_BODY_LIMIT]) await _send_notification( - summary=f"{len(new_prs)} new PR review requests", + summary=f"{len(filtered)} new PR review requests", body=body, urgency=urgency, ) +async def _notify_grouped_by_repo( + new_prs: list[PullRequest], + *, + threshold: int, + urgency: str, + repo_overrides: dict[str, RepoNotificationConfig] | None, +) -> None: + """Send notifications grouped by repository.""" + filtered = _filter_disabled_repos(new_prs, repo_overrides) + if not filtered: + return + + sorted_prs = sorted(filtered, key=lambda pr: pr.repo_full_name) + + async with aiohttp.ClientSession() as session: + for repo_name, group in itertools.groupby(sorted_prs, key=lambda pr: pr.repo_full_name): + repo_prs = list(group) + repo_threshold = _get_repo_threshold(repo_name, threshold, repo_overrides) + repo_urgency = _get_repo_urgency(repo_name, urgency, repo_overrides) + + if len(repo_prs) <= repo_threshold: + for pr in repo_prs: + icon = await _download_avatar(pr.author_avatar_url, session) + await _send_notification( + summary=f"PR Review: {pr.repo_full_name}", + body=f"#{pr.number} {pr.title}\nby {pr.author}", + url=pr.url, + icon=icon, + urgency=repo_urgency, + ) + else: + body = "\n".join(f"- #{pr.number}: {pr.title}" for pr in repo_prs[:_BATCH_BODY_LIMIT]) + await _send_notification( + summary=f"{len(repo_prs)} new PRs in {repo_name}", + body=body, + urgency=repo_urgency, + ) + + +def _filter_disabled_repos( + prs: list[PullRequest], + repo_overrides: dict[str, RepoNotificationConfig] | None, +) -> list[PullRequest]: + """Remove PRs from repos that have notifications disabled.""" + if not repo_overrides: + return prs + return [pr for pr in prs if repo_overrides.get(pr.repo_full_name, _DEFAULT_REPO_OVERRIDE).enabled] + + +def _get_repo_urgency( + repo_name: str, + default_urgency: str, + repo_overrides: dict[str, RepoNotificationConfig] | None, +) -> str: + """Return the urgency for a repo, falling back to the global default.""" + if not repo_overrides or repo_name not in repo_overrides: + return default_urgency + return repo_overrides[repo_name].urgency + + +def _get_repo_threshold( + repo_name: str, + default_threshold: int, + repo_overrides: dict[str, RepoNotificationConfig] | None, +) -> int: + """Return the threshold for a repo, falling back to the global default.""" + if not repo_overrides or repo_name not in repo_overrides: + return default_threshold + return repo_overrides[repo_name].threshold + + async def _send_notification( summary: str, body: str, diff --git a/tests/test_config.py b/tests/test_config.py index e8819fe..8bc4789 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -382,6 +382,163 @@ def test_poll_interval_at_boundary(tmp_path: Path) -> None: assert cfg.poll_interval == 30 +# --------------------------------------------------------------------------- +# Notification config — [notifications] section +# --------------------------------------------------------------------------- + + +def test_notifications_defaults_when_missing(minimal_config_file: Path) -> None: + """Missing [notifications] section should use defaults.""" + cfg = load_config(minimal_config_file) + assert cfg.notifications.grouping == "flat" + assert cfg.notifications.repos == {} + + +def test_notifications_grouping_flat(tmp_path: Path) -> None: + p = tmp_path / "config.toml" + p.write_text('github_token = "ghp_abc"\ngithub_username = "user"\n[notifications]\ngrouping = "flat"\n') + cfg = load_config(p) + assert cfg.notifications.grouping == "flat" + + +def test_notifications_grouping_repo(tmp_path: Path) -> None: + p = tmp_path / "config.toml" + p.write_text('github_token = "ghp_abc"\ngithub_username = "user"\n[notifications]\ngrouping = "repo"\n') + cfg = load_config(p) + assert cfg.notifications.grouping == "repo" + + +def test_notifications_grouping_invalid(tmp_path: Path) -> None: + p = tmp_path / "config.toml" + p.write_text('github_token = "ghp_abc"\ngithub_username = "user"\n[notifications]\ngrouping = "custom"\n') + with pytest.raises(ConfigError, match=r"notifications\.grouping must be one of"): + load_config(p) + + +def test_notifications_grouping_wrong_type(tmp_path: Path) -> None: + p = tmp_path / "config.toml" + p.write_text('github_token = "ghp_abc"\ngithub_username = "user"\n[notifications]\ngrouping = 42\n') + with pytest.raises(ConfigError, match=r"notifications\.grouping must be a string"): + load_config(p) + + +def test_notifications_grouping_case_insensitive(tmp_path: Path) -> None: + p = tmp_path / "config.toml" + p.write_text('github_token = "ghp_abc"\ngithub_username = "user"\n[notifications]\ngrouping = "Repo"\n') + cfg = load_config(p) + assert cfg.notifications.grouping == "repo" + + +def test_notifications_per_repo_full_config(tmp_path: Path) -> None: + """Full per-repo config with all fields.""" + p = tmp_path / "config.toml" + p.write_text( + 'github_token = "ghp_abc"\ngithub_username = "user"\n' + '[notifications]\ngrouping = "repo"\n' + '[notifications.repos."owner/repo"]\n' + "enabled = true\n" + 'urgency = "critical"\n' + "threshold = 5\n" + ) + cfg = load_config(p) + assert cfg.notifications.grouping == "repo" + repo_cfg = cfg.notifications.repos["owner/repo"] + assert repo_cfg.enabled is True + assert repo_cfg.urgency == "critical" + assert repo_cfg.threshold == 5 + + +def test_notifications_per_repo_disabled(tmp_path: Path) -> None: + p = tmp_path / "config.toml" + p.write_text( + 'github_token = "ghp_abc"\ngithub_username = "user"\n[notifications.repos."org/other-repo"]\nenabled = false\n' + ) + cfg = load_config(p) + repo_cfg = cfg.notifications.repos["org/other-repo"] + assert repo_cfg.enabled is False + assert repo_cfg.urgency == "normal" + assert repo_cfg.threshold == 3 + + +def test_notifications_per_repo_invalid_urgency(tmp_path: Path) -> None: + p = tmp_path / "config.toml" + p.write_text( + 'github_token = "ghp_abc"\ngithub_username = "user"\n[notifications.repos."owner/repo"]\nurgency = "urgent"\n' + ) + with pytest.raises(ConfigError, match=r"notifications\.repos\.'owner/repo'\.urgency must be one of"): + load_config(p) + + +def test_notifications_per_repo_invalid_threshold(tmp_path: Path) -> None: + p = tmp_path / "config.toml" + p.write_text( + 'github_token = "ghp_abc"\ngithub_username = "user"\n[notifications.repos."owner/repo"]\nthreshold = 0\n' + ) + with pytest.raises(ConfigError, match=r"notifications\.repos\.'owner/repo'\.threshold must be >= 1"): + load_config(p) + + +def test_notifications_per_repo_threshold_wrong_type(tmp_path: Path) -> None: + p = tmp_path / "config.toml" + p.write_text( + 'github_token = "ghp_abc"\ngithub_username = "user"\n[notifications.repos."owner/repo"]\nthreshold = "high"\n' + ) + with pytest.raises(ConfigError, match=r"notifications\.repos\.'owner/repo'\.threshold must be an integer"): + load_config(p) + + +def test_notifications_per_repo_enabled_wrong_type(tmp_path: Path) -> None: + p = tmp_path / "config.toml" + p.write_text( + 'github_token = "ghp_abc"\ngithub_username = "user"\n[notifications.repos."owner/repo"]\nenabled = "yes"\n' + ) + with pytest.raises(ConfigError, match=r"notifications\.repos\.'owner/repo'\.enabled must be a boolean"): + load_config(p) + + +def test_notifications_per_repo_urgency_wrong_type(tmp_path: Path) -> None: + p = tmp_path / "config.toml" + p.write_text( + 'github_token = "ghp_abc"\ngithub_username = "user"\n[notifications.repos."owner/repo"]\nurgency = 42\n' + ) + with pytest.raises(ConfigError, match=r"notifications\.repos\.'owner/repo'\.urgency must be a string"): + load_config(p) + + +def test_notifications_not_a_table(tmp_path: Path) -> None: + p = tmp_path / "config.toml" + p.write_text('github_token = "ghp_abc"\ngithub_username = "user"\nnotifications = "invalid"\n') + with pytest.raises(ConfigError, match="notifications must be a table"): + load_config(p) + + +def test_notifications_multiple_repos(tmp_path: Path) -> None: + """Multiple repos can have independent overrides.""" + p = tmp_path / "config.toml" + p.write_text( + 'github_token = "ghp_abc"\ngithub_username = "user"\n' + '[notifications]\ngrouping = "repo"\n' + '[notifications.repos."acme/web"]\n' + 'urgency = "critical"\n' + "threshold = 10\n" + '[notifications.repos."acme/api"]\n' + "enabled = false\n" + ) + cfg = load_config(p) + assert cfg.notifications.repos["acme/web"].urgency == "critical" + assert cfg.notifications.repos["acme/web"].threshold == 10 + assert cfg.notifications.repos["acme/api"].enabled is False + + +def test_notifications_urgency_case_insensitive(tmp_path: Path) -> None: + p = tmp_path / "config.toml" + p.write_text( + 'github_token = "ghp_abc"\ngithub_username = "user"\n[notifications.repos."owner/repo"]\nurgency = "Critical"\n' + ) + cfg = load_config(p) + assert cfg.notifications.repos["owner/repo"].urgency == "critical" + + # --------------------------------------------------------------------------- # Unknown keys warning (Step 3) # --------------------------------------------------------------------------- diff --git a/tests/test_daemon.py b/tests/test_daemon.py index b52f68d..7d026a5 100644 --- a/tests/test_daemon.py +++ b/tests/test_daemon.py @@ -151,6 +151,8 @@ async def test_notifies_on_new_prs(self) -> None: prs, threshold=3, urgency="normal", + grouping="flat", + repo_overrides=None, ) async def test_suppresses_notifications_on_first_poll(self) -> None: @@ -848,6 +850,8 @@ async def test_custom_threshold_and_urgency_passed(self) -> None: prs, threshold=10, urgency="critical", + grouping="flat", + repo_overrides=None, ) @@ -960,3 +964,74 @@ async def test_reload_passes_base_url_and_max_retries(self) -> None: assert daemon.client._base_url == "https://gh.new.example.com" assert daemon.client._max_retries == 7 + + +# --------------------------------------------------------------------------- +# Tests: notification grouping passthrough +# --------------------------------------------------------------------------- + + +class TestNotificationGroupingPassthrough: + """Daemon passes notification grouping and repo overrides to notify_new_prs.""" + + async def test_repo_grouping_mode_passed(self) -> None: + from forgewatch.config import NotificationConfig + + config = _make_config(notifications=NotificationConfig(grouping="repo")) + daemon = Daemon(config) + prs = [_make_pr(1)] + daemon.client.fetch_all = AsyncMock(return_value=prs) # type: ignore[method-assign] + daemon.interface = MagicMock() + daemon._first_poll = False + + with patch("forgewatch.daemon.notify_new_prs", new_callable=AsyncMock) as mock_notify: + await daemon._poll_once() + mock_notify.assert_awaited_once_with( + prs, + threshold=3, + urgency="normal", + grouping="repo", + repo_overrides=None, + ) + + async def test_repo_overrides_passed(self) -> None: + from forgewatch.config import NotificationConfig, RepoNotificationConfig + + overrides = {"acme/web": RepoNotificationConfig(urgency="critical", threshold=5)} + config = _make_config(notifications=NotificationConfig(grouping="repo", repos=overrides)) + daemon = Daemon(config) + prs = [_make_pr(1)] + daemon.client.fetch_all = AsyncMock(return_value=prs) # type: ignore[method-assign] + daemon.interface = MagicMock() + daemon._first_poll = False + + with patch("forgewatch.daemon.notify_new_prs", new_callable=AsyncMock) as mock_notify: + await daemon._poll_once() + mock_notify.assert_awaited_once_with( + prs, + threshold=3, + urgency="normal", + grouping="repo", + repo_overrides=overrides, + ) + + async def test_empty_repo_overrides_passes_none(self) -> None: + """When repos dict is empty, repo_overrides should be None.""" + from forgewatch.config import NotificationConfig + + config = _make_config(notifications=NotificationConfig(grouping="flat", repos={})) + daemon = Daemon(config) + prs = [_make_pr(1)] + daemon.client.fetch_all = AsyncMock(return_value=prs) # type: ignore[method-assign] + daemon.interface = MagicMock() + daemon._first_poll = False + + with patch("forgewatch.daemon.notify_new_prs", new_callable=AsyncMock) as mock_notify: + await daemon._poll_once() + mock_notify.assert_awaited_once_with( + prs, + threshold=3, + urgency="normal", + grouping="flat", + repo_overrides=None, + ) diff --git a/tests/test_notifier.py b/tests/test_notifier.py index cbee675..45be509 100644 --- a/tests/test_notifier.py +++ b/tests/test_notifier.py @@ -861,3 +861,344 @@ async def test_value_error_does_not_propagate(self) -> None: # Should not raise await _wait_and_open(proc, "https://github.com/owner/repo/pull/1") + + +# --------------------------------------------------------------------------- +# Tests: notify_new_prs — repo grouping mode +# --------------------------------------------------------------------------- + + +class TestNotifyRepoGrouping: + """notify_new_prs with grouping='repo' groups by repository.""" + + async def test_repo_grouping_individual_single_repo(self) -> None: + """Repo mode with PRs from one repo below threshold -> individual notifications.""" + prs = [_make_pr(number=i, repo="acme/web") for i in range(1, 3)] + proc = _mock_process() + + with ( + patch("forgewatch.notifier._download_avatar", return_value=None), + patch( + "forgewatch.notifier.asyncio.create_subprocess_exec", + return_value=proc, + ) as mock_exec, + ): + await notify_new_prs(prs, grouping="repo") + + assert mock_exec.call_count == 2 + + async def test_repo_grouping_individual_multi_repo(self) -> None: + """Repo mode with PRs from multiple repos, all below threshold -> individual per repo.""" + prs = [ + _make_pr(number=1, repo="acme/web"), + _make_pr(number=2, repo="acme/api"), + _make_pr(number=3, repo="acme/web"), + ] + proc = _mock_process() + + with ( + patch("forgewatch.notifier._download_avatar", return_value=None), + patch( + "forgewatch.notifier.asyncio.create_subprocess_exec", + return_value=proc, + ) as mock_exec, + ): + await notify_new_prs(prs, grouping="repo") + + # 1 PR in acme/api (individual) + 2 PRs in acme/web (individual) = 3 calls + assert mock_exec.call_count == 3 + + async def test_repo_grouping_summary_for_large_group(self) -> None: + """Repo mode with many PRs from one repo -> repo-level summary.""" + prs = [_make_pr(number=i, repo="acme/web", title=f"PR {i}") for i in range(1, 6)] + proc = _mock_process() + + with patch( + "forgewatch.notifier.asyncio.create_subprocess_exec", + return_value=proc, + ) as mock_exec: + await notify_new_prs(prs, grouping="repo") + + # Should be a single summary notification + mock_exec.assert_called_once() + args = mock_exec.call_args[0] + assert "5 new PRs in acme/web" in args + + async def test_repo_grouping_mixed_threshold(self) -> None: + """Repo mode: one repo below threshold (individual), another above (summary).""" + prs = [ + _make_pr(number=1, repo="acme/api", title="API fix"), + _make_pr(number=2, repo="acme/web", title="Web fix 1"), + _make_pr(number=3, repo="acme/web", title="Web fix 2"), + _make_pr(number=4, repo="acme/web", title="Web fix 3"), + _make_pr(number=5, repo="acme/web", title="Web fix 4"), + ] + proc = _mock_process() + + with ( + patch("forgewatch.notifier._download_avatar", return_value=None), + patch( + "forgewatch.notifier.asyncio.create_subprocess_exec", + return_value=proc, + ) as mock_exec, + ): + await notify_new_prs(prs, grouping="repo") + + # acme/api: 1 PR -> individual (1 call) + # acme/web: 4 PRs -> summary (1 call) + assert mock_exec.call_count == 2 + + # Check the summary notification for acme/web + all_args = [call[0] for call in mock_exec.call_args_list] + summary_args = [a for a in all_args if any("4 new PRs in acme/web" in s for s in a)] + assert len(summary_args) == 1 + + async def test_repo_grouping_summary_body_contains_pr_info(self) -> None: + """Repo summary body should list PR numbers and titles.""" + prs = [_make_pr(number=i, repo="acme/web", title=f"Fix {i}") for i in range(1, 5)] + proc = _mock_process() + + with patch( + "forgewatch.notifier.asyncio.create_subprocess_exec", + return_value=proc, + ) as mock_exec: + await notify_new_prs(prs, grouping="repo") + + args = mock_exec.call_args[0] + body = args[-1] + assert "- #1: Fix 1" in body + assert "- #2: Fix 2" in body + + +class TestNotifyFlatGrouping: + """notify_new_prs with grouping='flat' preserves existing behaviour.""" + + async def test_flat_mode_individual_notifications(self) -> None: + prs = [_make_pr(number=i) for i in range(1, 3)] + proc = _mock_process() + + with ( + patch("forgewatch.notifier._download_avatar", return_value=None), + patch( + "forgewatch.notifier.asyncio.create_subprocess_exec", + return_value=proc, + ) as mock_exec, + ): + await notify_new_prs(prs, grouping="flat") + + assert mock_exec.call_count == 2 + + async def test_flat_mode_batch_notification(self) -> None: + prs = [_make_pr(number=i) for i in range(1, 6)] + proc = _mock_process() + + with patch( + "forgewatch.notifier.asyncio.create_subprocess_exec", + return_value=proc, + ) as mock_exec: + await notify_new_prs(prs, grouping="flat") + + mock_exec.assert_called_once() + args = mock_exec.call_args[0] + assert "5 new PR review requests" in args + + async def test_default_grouping_is_flat(self) -> None: + """Without specifying grouping, behaviour matches flat mode.""" + prs = [_make_pr(number=i) for i in range(1, 6)] + proc = _mock_process() + + with patch( + "forgewatch.notifier.asyncio.create_subprocess_exec", + return_value=proc, + ) as mock_exec: + await notify_new_prs(prs) + + mock_exec.assert_called_once() + args = mock_exec.call_args[0] + assert "5 new PR review requests" in args + + +# --------------------------------------------------------------------------- +# Tests: notify_new_prs — per-repo overrides +# --------------------------------------------------------------------------- + + +class TestNotifyRepoOverrides: + """Per-repo notification overrides (enabled, urgency, threshold).""" + + async def test_disabled_repo_skipped_flat_mode(self) -> None: + """Disabled repo PRs should not produce notifications in flat mode.""" + from forgewatch.config import RepoNotificationConfig + + prs = [ + _make_pr(number=1, repo="acme/web"), + _make_pr(number=2, repo="acme/api"), + ] + overrides = { + "acme/web": RepoNotificationConfig(enabled=False), + } + proc = _mock_process() + + with ( + patch("forgewatch.notifier._download_avatar", return_value=None), + patch( + "forgewatch.notifier.asyncio.create_subprocess_exec", + return_value=proc, + ) as mock_exec, + ): + await notify_new_prs(prs, grouping="flat", repo_overrides=overrides) + + # Only acme/api PR should trigger a notification + mock_exec.assert_called_once() + args = mock_exec.call_args[0] + assert "acme/api" in " ".join(args) + + async def test_disabled_repo_skipped_repo_mode(self) -> None: + """Disabled repo PRs should not produce notifications in repo mode.""" + from forgewatch.config import RepoNotificationConfig + + prs = [ + _make_pr(number=1, repo="acme/web"), + _make_pr(number=2, repo="acme/api"), + ] + overrides = { + "acme/web": RepoNotificationConfig(enabled=False), + } + proc = _mock_process() + + with ( + patch("forgewatch.notifier._download_avatar", return_value=None), + patch( + "forgewatch.notifier.asyncio.create_subprocess_exec", + return_value=proc, + ) as mock_exec, + ): + await notify_new_prs(prs, grouping="repo", repo_overrides=overrides) + + mock_exec.assert_called_once() + args = mock_exec.call_args[0] + assert "acme/api" in " ".join(args) + + async def test_all_repos_disabled_sends_nothing(self) -> None: + """If all repos are disabled, no notifications are sent.""" + from forgewatch.config import RepoNotificationConfig + + prs = [ + _make_pr(number=1, repo="acme/web"), + _make_pr(number=2, repo="acme/api"), + ] + overrides = { + "acme/web": RepoNotificationConfig(enabled=False), + "acme/api": RepoNotificationConfig(enabled=False), + } + + with patch( + "forgewatch.notifier.asyncio.create_subprocess_exec", + ) as mock_exec: + await notify_new_prs(prs, grouping="flat", repo_overrides=overrides) + mock_exec.assert_not_called() + + async def test_per_repo_urgency_in_flat_mode(self) -> None: + """Per-repo urgency override should be used for individual notifications.""" + from forgewatch.config import RepoNotificationConfig + + pr = _make_pr(number=1, repo="acme/web") + overrides = { + "acme/web": RepoNotificationConfig(urgency="critical"), + } + proc = _mock_process() + + with ( + patch("forgewatch.notifier._download_avatar", return_value=None), + patch( + "forgewatch.notifier.asyncio.create_subprocess_exec", + return_value=proc, + ) as mock_exec, + ): + await notify_new_prs([pr], grouping="flat", urgency="low", repo_overrides=overrides) + + args = mock_exec.call_args[0] + assert "--urgency=critical" in args + + async def test_per_repo_urgency_in_repo_mode(self) -> None: + """Per-repo urgency override in repo grouping mode.""" + from forgewatch.config import RepoNotificationConfig + + pr = _make_pr(number=1, repo="acme/web") + overrides = { + "acme/web": RepoNotificationConfig(urgency="critical"), + } + proc = _mock_process() + + with ( + patch("forgewatch.notifier._download_avatar", return_value=None), + patch( + "forgewatch.notifier.asyncio.create_subprocess_exec", + return_value=proc, + ) as mock_exec, + ): + await notify_new_prs([pr], grouping="repo", urgency="low", repo_overrides=overrides) + + args = mock_exec.call_args[0] + assert "--urgency=critical" in args + + async def test_per_repo_threshold_in_repo_mode(self) -> None: + """Per-repo threshold override — repo with threshold=1 should summarise 2 PRs.""" + from forgewatch.config import RepoNotificationConfig + + prs = [ + _make_pr(number=1, repo="acme/web"), + _make_pr(number=2, repo="acme/web"), + ] + overrides = { + "acme/web": RepoNotificationConfig(threshold=1), + } + proc = _mock_process() + + with patch( + "forgewatch.notifier.asyncio.create_subprocess_exec", + return_value=proc, + ) as mock_exec: + await notify_new_prs(prs, grouping="repo", repo_overrides=overrides) + + mock_exec.assert_called_once() + args = mock_exec.call_args[0] + assert "2 new PRs in acme/web" in args + + async def test_repos_without_overrides_use_global_defaults(self) -> None: + """Repos not in overrides should use global urgency/threshold.""" + from forgewatch.config import RepoNotificationConfig + + pr = _make_pr(number=1, repo="acme/other") + overrides = { + "acme/web": RepoNotificationConfig(urgency="critical"), + } + proc = _mock_process() + + with ( + patch("forgewatch.notifier._download_avatar", return_value=None), + patch( + "forgewatch.notifier.asyncio.create_subprocess_exec", + return_value=proc, + ) as mock_exec, + ): + await notify_new_prs([pr], grouping="repo", urgency="normal", repo_overrides=overrides) + + args = mock_exec.call_args[0] + assert "--urgency=normal" in args + + async def test_none_overrides_behaves_like_no_overrides(self) -> None: + """repo_overrides=None should behave identically to no overrides.""" + prs = [_make_pr(number=1)] + proc = _mock_process() + + with ( + patch("forgewatch.notifier._download_avatar", return_value=None), + patch( + "forgewatch.notifier.asyncio.create_subprocess_exec", + return_value=proc, + ) as mock_exec, + ): + await notify_new_prs(prs, repo_overrides=None) + + mock_exec.assert_called_once() From 39873cd049d5efe51b54d4cfac185df4b8b2f059 Mon Sep 17 00:00:00 2001 From: Jan Dvorak Date: Fri, 13 Mar 2026 11:55:46 +0100 Subject: [PATCH 4/4] add shell completion support via shtab --- .github/CONTRIBUTING.md | 2 +- AGENTS.md | 23 ++++++++++++++-------- CHANGELOG.md | 1 + README.md | 5 ++++- docs/architecture.md | 22 +++++++++++++-------- docs/development.md | 2 +- docs/modules/cli.md | 40 ++++++++++++++++++++++---------------- docs/modules/daemon.md | 12 +++++++----- forgewatch/__main__.py | 15 ++++++++++---- forgewatch/cli/__init__.py | 26 +++++++++++++++++++++---- pyproject.toml | 1 + tests/test_cli_init.py | 40 ++++++++++++++++++++++++++++++++++++++ tests/test_main.py | 6 +++--- uv.lock | 11 +++++++++++ 14 files changed, 154 insertions(+), 52 deletions(-) diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index c8f671e..c3a34bc 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -107,7 +107,7 @@ forgewatch/ dbus_service.py D-Bus interface (dbus-next) config.py TOML config loading + validation url_opener.py Shared URL opener (XDG portal + xdg-open) - cli/ Management subcommands (setup, service, uninstall) + cli/ Management subcommands (setup, service, uninstall, completions) indicator/ System tray icon + popup window (GTK3, separate process) ``` diff --git a/AGENTS.md b/AGENTS.md index 96eabab..4940cac 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -18,7 +18,7 @@ Key traits: ``` __main__.py CLI entry point -- dispatches to CLI subcommands or daemon | - +---> cli/ Management subcommands (setup, service, uninstall) + +---> cli/ Management subcommands (setup, service, uninstall, completions) | __init__.py Argparse parser + run_cli() dispatch | setup.py Interactive setup wizard (config + systemd) | service.py Systemd service management (start/stop/status/...) @@ -56,7 +56,7 @@ indicator/ Separate process -- system tray icon + popup window | Module | Role | |---|---| -| `__main__.py` | CLI entry point. Builds a unified argparse parser with daemon flags (`-c`, `-v`) and management subcommands (`setup`, `service`, `uninstall`). When `args.command` is set, dispatches to `cli.dispatch()`. Otherwise starts the daemon. | +| `__main__.py` | CLI entry point. Builds a unified argparse parser with daemon flags (`-c`, `-v`) and management subcommands (`setup`, `service`, `uninstall`, `completions`). When `args.command` is set, dispatches to `cli.dispatch()`. Otherwise starts the daemon. Also integrates `shtab` for shell-completion metadata on the `--config` flag. | | `cli/__init__.py` | Registers subcommands via `add_subcommands()`, dispatches via `dispatch()`. Also provides `build_parser()` and `run_cli()` for standalone/test use. | | `cli/setup.py` | Interactive setup wizard. Config file creation (token, username, poll interval, repos), systemd service installation, and enable+start. Supports `--config-only` and `--service-only` flags. | | `cli/service.py` | Thin CLI layer over `_systemd.py`. Actions: `install`, `start`, `stop`, `restart`, `status`, `enable`, `disable`. Manages both daemon and indicator services. | @@ -118,6 +118,9 @@ uv run forgewatch service install # install systemd unit files uv run forgewatch service enable # enable autostart uv run forgewatch service disable # disable autostart uv run forgewatch uninstall # remove services + optionally config +uv run forgewatch completions bash # generate bash completions +uv run forgewatch completions zsh # generate zsh completions +uv run forgewatch completions tcsh # generate tcsh completions # Install as systemd user service systemctl --user enable --now forgewatch @@ -199,7 +202,7 @@ uv run mypy forgewatch | `systemd/forgewatch.service` | Systemd user unit file for the daemon. | | `systemd/forgewatch-indicator.service` | Systemd user unit file for the indicator. Depends on the daemon service. | | `forgewatch/indicator/` | System tray indicator package. Separate process, connects to daemon over D-Bus. Requires GTK3/AppIndicator3/gbulb. | -| `forgewatch/cli/` | CLI management subcommands package. Setup wizard, service management, uninstall. Stdlib only (no extra deps). | +| `forgewatch/cli/` | CLI management subcommands package. Setup wizard, service management, uninstall, shell completions. The `completions` subcommand uses `shtab`; all other subcommands are stdlib only. | | `forgewatch/cli/systemd/` | Bundled `.service` files accessed via `importlib.resources`. | | `forgewatch/url_opener.py` | Shared URL opener (XDG portal + xdg-open fallback). Used by both notifier and indicator. | | `docs/` | Architecture, configuration, development, and module documentation. | @@ -254,19 +257,23 @@ in `dbus_service.py`), also update: ### Adding/modifying CLI subcommands -The CLI package (`forgewatch.cli`) uses stdlib only (no extra deps). Key patterns: +The CLI package (`forgewatch.cli`) uses stdlib only (no extra deps) for most +subcommands. The `completions` subcommand uses `shtab`. Key patterns: 1. **Shared helpers** are in `_output.py`, `_prompts.py`, `_checks.py`, and `_systemd.py`. These are pure-Python modules with no async code. 2. **Subcommand handlers** are in `setup.py`, `service.py`, and `uninstall.py`. - Each exports a single `run_*()` entry point. + Each exports a single `run_*()` entry point. The `completions` subcommand + is handled inline in `dispatch()` via `shtab.complete()`. 3. **Parser and dispatch** are in `__init__.py` (`add_subcommands()` + `dispatch()`). `build_parser()` and `run_cli()` are also provided for standalone/test use. Subcommand modules are imported lazily inside `dispatch()` to avoid loading unused code. -4. **Unified parser** in `__main__.py` builds a single argparse parser with both - daemon flags (`-c`, `-v`) and management subcommands. When `args.command` is - not `None`, it dispatches to `cli.dispatch(args)`. +4. **Unified parser** in `__main__.py` (`build_full_parser()`) builds a single + argparse parser with both daemon flags (`-c`, `-v`) and management + subcommands. It also attaches `shtab.FILE` completion metadata to the + `--config` flag. When `args.command` is not `None`, it dispatches to + `cli.dispatch(args)`. 5. **Bundled service files** live in `cli/systemd/` and are read via `importlib.resources.files("forgewatch.cli.systemd")`. diff --git a/CHANGELOG.md b/CHANGELOG.md index c1fb814..26db42d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Shell completion support -- new `forgewatch completions ` subcommand generates Bash, Zsh, or tcsh completions via `shtab`, with file-path completion for `--config` - Pagination cap warning -- the poller now logs a warning when the page limit is reached and more results are available, suggesting the user narrow their repo filter - Configurable indicator settings via a new `[indicator]` TOML section -- `reconnect_interval`, `window_width`, and `max_window_height` can now be tuned without code changes - Unknown config key warnings -- `load_config()` now logs a warning for any unrecognised top-level key, helping catch typos early diff --git a/README.md b/README.md index 7222793..837937a 100644 --- a/README.md +++ b/README.md @@ -214,6 +214,9 @@ forgewatch service start|stop|restart forgewatch service install # install systemd unit files forgewatch service enable|disable # toggle autostart forgewatch uninstall # remove services + optionally config +forgewatch completions bash # generate bash completions +forgewatch completions zsh # generate zsh completions +forgewatch completions tcsh # generate tcsh completions ``` --- @@ -288,7 +291,7 @@ patterns, CI pipeline details, and project structure. | Module | Description | |---|---| -| [CLI](docs/modules/cli.md) | Management subcommands (setup, service, uninstall) | +| [CLI](docs/modules/cli.md) | Management subcommands (setup, service, uninstall, completions) | | [Config](docs/modules/config.md) | Configuration loading and validation | | [Poller](docs/modules/poller.md) | GitHub API client, pagination, rate limiting | | [Store](docs/modules/store.md) | In-memory state store with diff computation | diff --git a/docs/architecture.md b/docs/architecture.md index 61c9980..7757110 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -161,10 +161,11 @@ See [modules/indicator.md](modules/indicator.md) for the full API reference. ### CLI Management (`cli/`) -A management interface providing `setup`, `service`, and `uninstall` -subcommands for installing and managing ForgeWatch as a systemd user -service. Uses stdlib only (no extra dependencies beyond the Python standard -library). The package consists of: +A management interface providing `setup`, `service`, `uninstall`, and +`completions` subcommands for installing and managing ForgeWatch as a +systemd user service. Uses stdlib only for most subcommands; the +`completions` subcommand uses `shtab` for shell-completion generation. +The package consists of: - **Parser and dispatch** (`__init__.py`) -- argparse subcommand parser with lazy imports to avoid loading unused code. @@ -184,10 +185,15 @@ library). The package consists of: - **Bundled service files** (`systemd/`) -- `.service` files accessed via `importlib.resources`, allowing installation from PyPI packages without a git checkout. - -Subcommand detection happens in `__main__.py` by checking `sys.argv[1]` -against a known set of command names before the daemon argparse runs, ensuring -full backward compatibility with existing daemon flags (`-c`, `-v`). +- **Shell completions** -- the `completions` subcommand generates Bash, Zsh, + or tcsh completion scripts via `shtab`. The unified parser in `__main__.py` + (`build_full_parser()`) attaches `shtab.FILE` metadata to the `--config` + flag for file-path completion. + +Subcommand detection happens in `__main__.py` via the unified argparse +parser. When `args.command` is not `None`, the request is dispatched to +`cli.dispatch(args)`. Otherwise the daemon starts, ensuring full backward +compatibility with existing daemon flags (`-c`, `-v`). See [modules/cli.md](modules/cli.md) for the full API reference. diff --git a/docs/development.md b/docs/development.md index 6c129e6..29632a5 100644 --- a/docs/development.md +++ b/docs/development.md @@ -32,7 +32,7 @@ forgewatch/ # Main package ├── notifier.py # Desktop notifications ├── url_opener.py # Shared URL opener (XDG portal + xdg-open) ├── daemon.py # Main daemon loop -├── cli/ # CLI management subcommands (stdlib only) +├── cli/ # CLI management subcommands (stdlib + shtab) │ ├── __init__.py # Subcommand parser + dispatch │ ├── setup.py # Setup wizard (config + systemd) │ ├── service.py # Service management (start/stop/status/...) diff --git a/docs/modules/cli.md b/docs/modules/cli.md index d9b73dd..267198d 100644 --- a/docs/modules/cli.md +++ b/docs/modules/cli.md @@ -3,8 +3,8 @@ Package: `forgewatch.cli` CLI management subcommands for installing and managing ForgeWatch as a -systemd user service. This package uses **stdlib only** -- no extra dependencies -beyond the Python standard library. +systemd user service. Most subcommands use **stdlib only**; the `completions` +subcommand uses `shtab` for shell-completion generation. ## Command overview @@ -12,13 +12,14 @@ beyond the Python standard library. forgewatch setup [--config-only | --service-only] forgewatch service {install,start,stop,restart,status,enable,disable} forgewatch uninstall +forgewatch completions {bash,zsh,tcsh} ``` When the first argument to `forgewatch` is a known subcommand (`setup`, -`service`, `uninstall`), the unified parser in `__main__.py` dispatches to -`cli.dispatch()`. Otherwise, the daemon starts as usual for full backward -compatibility. Running `forgewatch --help` shows both daemon flags and -management subcommands. +`service`, `uninstall`, `completions`), the unified parser in `__main__.py` +dispatches to `cli.dispatch()`. Otherwise, the daemon starts as usual for +full backward compatibility. Running `forgewatch --help` shows both daemon +flags and management subcommands. ## Module structure @@ -44,7 +45,7 @@ cli/ ### `add_subcommands(subparsers) -> None` Add CLI management subcommands to an existing subparsers group. This is called -by `__main__._build_parser()` to register subcommands on the unified parser. +by `__main__.build_full_parser()` to register subcommands on the unified parser. | Parameter | Type | Description | |---|---|---| @@ -66,6 +67,7 @@ Dispatch targets (lazy-imported inside the function body): | `setup` | `setup.run_setup(config_only=..., service_only=...)` | | `service` | `service.run_service(action=...)` | | `uninstall` | `uninstall.run_uninstall()` | +| `completions` | `shtab.complete(parser, args.shell)` (inline, uses `build_full_parser()`) | ### `build_parser() -> argparse.ArgumentParser` @@ -73,13 +75,14 @@ Build a standalone argparse parser for CLI management subcommands. Uses `add_subcommands()` internally. This parser does **not** include daemon flags (`-c`, `-v`) -- those live on the unified parser in `__main__.py`. -Returns an `ArgumentParser` with three subcommands: +Returns an `ArgumentParser` with four subcommands: | Subcommand | Arguments | Description | |---|---|---| | `setup` | `--config-only`, `--service-only` (mutually exclusive) | Interactive setup wizard | | `service` | `action` (positional, choices: `install`, `start`, `stop`, `restart`, `status`, `enable`, `disable`) | Manage systemd services | | `uninstall` | *(none)* | Remove services and optionally config | +| `completions` | `shell` (positional, choices: `bash`, `zsh`, `tcsh`) | Generate shell completions | ### `run_cli(argv: list[str] | None = None) -> None` @@ -533,18 +536,21 @@ cleans up entries created by older versions of the install script. ## Design notes -- **Stdlib only:** The entire CLI package uses only the Python standard library - (`argparse`, `subprocess`, `shutil`, `pathlib`, `importlib.resources`). This - keeps the dependency footprint minimal and ensures the management commands - work even if optional runtime dependencies (aiohttp, dbus-next) fail to - import. +- **Mostly stdlib:** The CLI package uses the Python standard library + (`argparse`, `subprocess`, `shutil`, `pathlib`, `importlib.resources`) for + most subcommands. The `completions` subcommand uses `shtab` for + shell-completion generation. This keeps the dependency footprint minimal + and ensures the management commands work even if optional runtime + dependencies (aiohttp, dbus-next) fail to import. - **Lazy imports:** Subcommand modules (`setup.py`, `service.py`, `uninstall.py`) are imported lazily inside `run_cli()` to avoid loading - unused code when only one subcommand is invoked. + unused code when only one subcommand is invoked. `shtab` is also imported + lazily inside `dispatch()` and `build_full_parser()`. - **Subcommand detection:** Happens in `__main__.py` via the unified argparse - parser. When `args.command` is not `None`, the request is dispatched to - `cli.dispatch(args)`. Otherwise the daemon starts. This avoids conflicts - between daemon flags (`-c`, `-v`) and subcommand names. + parser (`build_full_parser()`). When `args.command` is not `None`, the + request is dispatched to `cli.dispatch(args)`. Otherwise the daemon starts. + This avoids conflicts between daemon flags (`-c`, `-v`) and subcommand + names. - **Separate stdout/stderr TTY detection:** `_output.py` uses two separate `isatty()` checks because `warn()` and `err()` write to stderr (which may have different TTY status than stdout when piping). diff --git a/docs/modules/daemon.md b/docs/modules/daemon.md index 5aeeba8..e22e142 100644 --- a/docs/modules/daemon.md +++ b/docs/modules/daemon.md @@ -181,19 +181,20 @@ parsing and logging setup. The `main()` function in `__main__.py` is the single entry point for all `forgewatch` invocations. A unified argument parser exposes both daemon -flags (`-c`, `-v`) and management subcommands (`setup`, `service`, `uninstall`) -so that `forgewatch --help` shows everything: +flags (`-c`, `-v`) and management subcommands (`setup`, `service`, `uninstall`, +`completions`) so that `forgewatch --help` shows everything: ``` -usage: forgewatch [-h] [-c CONFIG] [-v] {setup,service,uninstall} ... +usage: forgewatch [-h] [-c CONFIG] [-v] {setup,service,uninstall,completions} ... ForgeWatch — GitHub PR Monitor positional arguments: - {setup,service,uninstall} + {setup,service,uninstall,completions} setup Initial setup wizard service Manage systemd services uninstall Remove services and optionally config + completions Generate shell completions options: -h, --help show this help message and exit @@ -203,7 +204,7 @@ options: Run without a command to start the daemon. ``` -The parser is built by `_build_parser()`, which calls +The parser is built by `build_full_parser()`, which calls `cli.add_subcommands()` to register the management subcommands. After parsing, `main()` checks `args.command`: @@ -218,6 +219,7 @@ forgewatch setup --config-only # only create config.toml forgewatch setup --service-only # only install + start systemd services forgewatch service # start | stop | restart | status | install | enable | disable forgewatch uninstall # remove services, optionally remove config +forgewatch completions # generate shell completions (bash, zsh, tcsh) ``` See [cli module docs](cli.md) for the full API reference. diff --git a/forgewatch/__main__.py b/forgewatch/__main__.py index dc5d300..e27e525 100644 --- a/forgewatch/__main__.py +++ b/forgewatch/__main__.py @@ -13,12 +13,18 @@ import argparse -def _build_parser() -> argparse.ArgumentParser: +def build_full_parser() -> argparse.ArgumentParser: """Build the unified argument parser. Top-level flags (``-c``, ``-v``) are for daemon mode. Subcommands - (``setup``, ``service``, ``uninstall``) are for management tasks. + (``setup``, ``service``, ``uninstall``, ``completions``) are for + management tasks. + + This function is public so that :func:`forgewatch.cli.dispatch` can + obtain the full parser for shell-completion generation via ``shtab``. """ + import shtab # noqa: PLC0415 + from .cli import add_subcommands # noqa: PLC0415 parser = argparse.ArgumentParser( @@ -26,13 +32,14 @@ def _build_parser() -> argparse.ArgumentParser: description="ForgeWatch — GitHub PR Monitor", epilog="Run without a command to start the daemon.", ) - parser.add_argument( + config_action = parser.add_argument( "-c", "--config", type=str, default=None, help="Path to config.toml", ) + config_action.complete = shtab.FILE # type: ignore[attr-defined] parser.add_argument( "-v", "--verbose", @@ -52,7 +59,7 @@ def main() -> None: Parses arguments with the unified parser and dispatches to the management CLI or starts the daemon. """ - parser = _build_parser() + parser = build_full_parser() args = parser.parse_args() if args.command is not None: diff --git a/forgewatch/cli/__init__.py b/forgewatch/cli/__init__.py index 4f5fd54..94b842a 100644 --- a/forgewatch/cli/__init__.py +++ b/forgewatch/cli/__init__.py @@ -20,7 +20,8 @@ def add_subcommands(subparsers: _SubParsersAction[argparse.ArgumentParser]) -> N ---------- subparsers: A subparsers action (from ``parser.add_subparsers()``) to which - ``setup``, ``service``, and ``uninstall`` subcommands are added. + ``setup``, ``service``, ``uninstall``, and ``completions`` + subcommands are added. """ # setup setup_parser = subparsers.add_parser("setup", help="Initial setup wizard") @@ -47,6 +48,14 @@ def add_subcommands(subparsers: _SubParsersAction[argparse.ArgumentParser]) -> N # uninstall subparsers.add_parser("uninstall", help="Remove services and optionally config") + # completions + comp = subparsers.add_parser("completions", help="Generate shell completions") + comp.add_argument( + "shell", + choices=["bash", "zsh", "tcsh"], + help="Shell to generate completions for", + ) + def dispatch(args: argparse.Namespace) -> None: """Dispatch to the appropriate CLI subcommand handler. @@ -72,13 +81,22 @@ def dispatch(args: argparse.Namespace) -> None: run_uninstall() + elif args.command == "completions": + import shtab # noqa: PLC0415 + + from forgewatch.__main__ import build_full_parser # noqa: PLC0415 + + parser = build_full_parser() + print(shtab.complete(parser, args.shell)) # noqa: T201 + def build_parser() -> argparse.ArgumentParser: """Build the argparse parser for CLI management subcommands. - Returns an ``ArgumentParser`` with ``setup``, ``service``, and - ``uninstall`` subcommands. This is used by :func:`run_cli` and by - tests that need to inspect the parser structure independently. + Returns an ``ArgumentParser`` with ``setup``, ``service``, + ``uninstall``, and ``completions`` subcommands. This is used by + :func:`run_cli` and by tests that need to inspect the parser + structure independently. """ parser = argparse.ArgumentParser( prog="forgewatch", diff --git a/pyproject.toml b/pyproject.toml index 409ce1b..94b0faf 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -38,6 +38,7 @@ dependencies = [ "aiohttp>=3.9,<4", "dbus-next>=0.2.3,<1", "gbulb>=0.6", + "shtab>=1.7", ] [project.urls] diff --git a/tests/test_cli_init.py b/tests/test_cli_init.py index fb1ae37..e2cb18e 100644 --- a/tests/test_cli_init.py +++ b/tests/test_cli_init.py @@ -80,6 +80,28 @@ def test_no_command_sets_none(self) -> None: args = parser.parse_args([]) assert args.command is None + def test_completions_subcommand_accepted(self) -> None: + parser = build_parser() + args = parser.parse_args(["completions", "bash"]) + assert args.command == "completions" + assert args.shell == "bash" + + def test_completions_all_shells_valid(self) -> None: + parser = build_parser() + for shell in ("bash", "zsh", "tcsh"): + args = parser.parse_args(["completions", shell]) + assert args.shell == shell + + def test_completions_invalid_shell_rejected(self) -> None: + parser = build_parser() + with pytest.raises(SystemExit): + parser.parse_args(["completions", "fish"]) + + def test_completions_missing_shell_rejected(self) -> None: + parser = build_parser() + with pytest.raises(SystemExit): + parser.parse_args(["completions"]) + # --------------------------------------------------------------------------- # run_cli — dispatch logic @@ -144,6 +166,24 @@ def test_dispatches_uninstall(self, mock_run_uninstall: MagicMock) -> None: run_cli(["uninstall"]) mock_run_uninstall.assert_called_once_with() + def test_dispatches_completions_bash(self, capsys: pytest.CaptureFixture[str]) -> None: + run_cli(["completions", "bash"]) + output = capsys.readouterr().out + assert len(output) > 0 + assert "forgewatch" in output + + def test_dispatches_completions_zsh(self, capsys: pytest.CaptureFixture[str]) -> None: + run_cli(["completions", "zsh"]) + output = capsys.readouterr().out + assert len(output) > 0 + assert "forgewatch" in output + + def test_dispatches_completions_tcsh(self, capsys: pytest.CaptureFixture[str]) -> None: + run_cli(["completions", "tcsh"]) + output = capsys.readouterr().out + assert len(output) > 0 + assert "forgewatch" in output + def test_no_command_prints_help_and_exits(self) -> None: with pytest.raises(SystemExit, match="1"): run_cli([]) diff --git a/tests/test_main.py b/tests/test_main.py index 246405f..b329cef 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -9,7 +9,7 @@ import pytest -from forgewatch.__main__ import _build_parser, main +from forgewatch.__main__ import build_full_parser, main from forgewatch.config import Config, ConfigError # --------------------------------------------------------------------------- @@ -213,14 +213,14 @@ class TestCliDispatch: def test_unified_help_includes_subcommands(self) -> None: """--help output should mention all management subcommands.""" - parser = _build_parser() + parser = build_full_parser() help_text = parser.format_help() assert "setup" in help_text assert "service" in help_text assert "uninstall" in help_text def test_unified_help_includes_daemon_flags(self) -> None: - parser = _build_parser() + parser = build_full_parser() help_text = parser.format_help() assert "--config" in help_text assert "--verbose" in help_text diff --git a/uv.lock b/uv.lock index 97eb194..492c437 100644 --- a/uv.lock +++ b/uv.lock @@ -448,6 +448,7 @@ dependencies = [ { name = "aiohttp" }, { name = "dbus-next" }, { name = "gbulb" }, + { name = "shtab" }, ] [package.dev-dependencies] @@ -468,6 +469,7 @@ requires-dist = [ { name = "aiohttp", specifier = ">=3.9,<4" }, { name = "dbus-next", specifier = ">=0.2.3,<1" }, { name = "gbulb", specifier = ">=0.6" }, + { name = "shtab", specifier = ">=1.7" }, ] [package.metadata.requires-dev] @@ -1413,6 +1415,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/fe/4e/cd76eca6db6115604b7626668e891c9dd03330384082e33662fb0f113614/ruff-0.15.5-py3-none-win_arm64.whl", hash = "sha256:b498d1c60d2fe5c10c45ec3f698901065772730b411f164ae270bb6bfcc4740b", size = 10965572, upload-time = "2026-03-05T20:06:16.984Z" }, ] +[[package]] +name = "shtab" +version = "1.8.0" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/b0/7a/7f131b6082d8b592c32e4312d0a6da3d0b28b8f0d305ddd93e49c9d89929/shtab-1.8.0.tar.gz", hash = "sha256:75f16d42178882b7f7126a0c2cb3c848daed2f4f5a276dd1ded75921cc4d073a", size = 46062, upload-time = "2025-11-18T10:57:47.601Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/8e/e1/202a31727b0d096a04380f78e809074d7a1d0a22d9d5a39fea1d2353fd02/shtab-1.8.0-py3-none-any.whl", hash = "sha256:f0922a82174b4007e06ac0bac4f79abd826c5cca88e201bfd927f889803c571d", size = 14457, upload-time = "2025-11-18T10:57:45.906Z" }, +] + [[package]] name = "sortedcontainers" version = "2.4.0"