diff --git a/CHANGELOG.md b/CHANGELOG.md index 5dcd52b..cae70b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,21 @@ 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 +- 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 ### Fixed 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/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/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/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/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/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/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/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_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 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" },