From 5022e2819edcbef05e3d7c4ae8fdd1fdd233ff2d Mon Sep 17 00:00:00 2001 From: txoof-bot <337660340+txoof-bot@users.noreply.github.com> Date: Wed, 7 Oct 2026 15:46:01 +0200 Subject: [PATCH 1/4] Required plugin settings (M5 part 2a) Plugins can mark a setting as required with paperpi.plugin.setting(..., required=True). A switched-on plugin that is missing one is not shown and not counted as broken; the config check warns and paperpi list shows "needs ". met_no and moon_phase require lat, lon and email (met.no's terms of service ask for a real email address); the example config no longer fills in a made-up address. Part of #238 Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- docs/decisions/plugin-interface.md | 2 + docs/writing-plugins.md | 9 ++++ paperpi.example.toml | 15 ++++--- src/paperpi/cli.py | 11 ++++- src/paperpi/config.py | 23 ++++++++++ src/paperpi/example.py | 13 +++--- src/paperpi/plugin.py | 52 +++++++++++++++++++++- src/paperpi/plugins/met_no/README.md | 6 +-- src/paperpi/plugins/met_no/__init__.py | 16 ++++--- src/paperpi/plugins/moon_phase/README.md | 6 +-- src/paperpi/plugins/moon_phase/__init__.py | 17 +++---- src/paperpi/scheduler.py | 2 +- tests/test_cli.py | 2 + tests/test_config.py | 46 +++++++++++++++++++ tests/test_example.py | 22 +++++++-- tests/test_plugin.py | 51 ++++++++++++++++++++- tests/test_scheduler.py | 14 ++++++ 18 files changed, 267 insertions(+), 42 deletions(-) diff --git a/README.md b/README.md index 8a003ed..4bfbf5a 100644 --- a/README.md +++ b/README.md @@ -68,7 +68,7 @@ Run `uv run paperpi render --help` for all options. How to write a plugin: [docs ### A config file -`paperpi example-config` prints an example config file that works as it is: a virtual screen, a clock, and the weather in Berlin and Rio. Every other setting is a comment with its default and a short help text. [paperpi.example.toml](paperpi.example.toml) is the same text. Before you use the weather blocks, put your own email address in `email` (met.no asks for it). `-o` doesn't replace a file that is already there, unless you add `--force`. +`paperpi example-config` prints an example config file that works as it is: a virtual screen, a clock, and the weather in Berlin and Rio. Every other setting is a comment with its default and a short help text. [paperpi.example.toml](paperpi.example.toml) is the same text. The weather blocks are not shown until you put your own, real email address in `email`: met.no's terms of service ask for it, so the example leaves it empty. `paperpi list` shows them as "needs email" until then. `-o` doesn't replace a file that is already there, unless you add `--force`. ```bash uv run paperpi example-config -o paperpi.toml diff --git a/docs/decisions/plugin-interface.md b/docs/decisions/plugin-interface.md index 60f0b73..0ba9457 100644 --- a/docs/decisions/plugin-interface.md +++ b/docs/decisions/plugin-interface.md @@ -50,6 +50,8 @@ The plugin never talks to the screen. Only the scheduler does. For sample images and tests, `fetch` is skipped and the sample data goes straight to `draw`. How to write a plugin: `docs/writing-plugins.md`. +**Update 2026-10-07 (M5 part 2a, issue #238, agreed with txoof):** a plugin can mark a setting as **required** with `setting(..., required=True)`, for settings it can't work without, such as an API key. The first ones are `lat`, `lon` and `email` of `met_no` and `moon_phase`: met.no's terms of service ask for a real email address, and a user should not start these plugins without giving one. A plugin that is missing a required setting can be in the config file and switched on, but it is **not shown and not counted as broken**: the config check gives a warning naming the missing settings, `paperpi list` shows "needs email" in its "on" column, and the web interface (part 2b) shows "Needs settings" in place of the on switch. The example config no longer fills in a made-up email address. `setting` is the one standard place where a setting asks PaperPi for help; the input helpers of `web-interface.md` (such as `location`, which looks up latitude and longitude; part 3c) will be options of `setting` too. + ### How a plugin is run - Every update runs in a **new, short-lived process**. When the update is done, the process exits and its memory is given back. A plugin that hangs or crashes is stopped without affecting PaperPi or the other plugins. diff --git a/docs/writing-plugins.md b/docs/writing-plugins.md index 604964a..2a0ace9 100644 --- a/docs/writing-plugins.md +++ b/docs/writing-plugins.md @@ -80,6 +80,15 @@ Each setting is a field of the plugin's `Settings` class, with a type, a default - Every setting needs a default. - Don't use the names of the shared settings, which every `[[plugin]]` block already has: `name`, `type`, `enabled`, `level`, `display_time`, `refresh`, `time_limit`, `layout`, `alert_reminder`, `alert_max_time`. - Use `pydantic.SecretStr` as the type for API keys and passwords. PaperPi then never shows their values in error messages or logs. +- A setting the user must fill in before the plugin can work (a place, an email address, an API key) is made with `paperpi.plugin.setting(..., required=True)` instead of `Field`. It takes the same arguments as `Field`. Its default must be "not set": `None` or `""` (or an empty `SecretStr`). Until every required setting is filled in, the plugin is not shown and not counted as broken; the config check, `paperpi list` and the web interface say which settings are missing. The example block marks them "(required)". `setting` is the one place where a setting asks PaperPi for more help; later options, such as a helper in the web interface that looks up latitude and longitude, are added there too. + + ```python + from paperpi.plugin import PluginSettings, setting + + + class Settings(PluginSettings): + lat: float | None = setting(None, required=True, ge=-90, le=90, description="Latitude") + ``` In the config file, the plugin's settings go in its `[[plugin]]` block: diff --git a/paperpi.example.toml b/paperpi.example.toml index 769fcca..b5a0c9a 100644 --- a/paperpi.example.toml +++ b/paperpi.example.toml @@ -4,8 +4,9 @@ # The file only needs the settings that differ from the default. Every other setting is # shown as a comment with its default: remove the # in front of it to change it. # Each [[plugin]] block is one plugin on the screen; the same plugin type can be used -# more than once, with a different name. The weather blocks need your own email address -# in "email": met.no asks every program for a way to contact its user. +# more than once, with a different name. The weather blocks are not shown until you fill +# in your own, real email address in "email": met.no asks every program for a way to +# contact its user. Settings marked "(required)" must be filled in before a plugin is shown. config_version = 1 @@ -97,8 +98,9 @@ lat = 52.52 lon = 13.4 # Name shown on the screen, e.g. Berlin (else lat, lon) place = "Berlin" -# Your email address, sent only to met.no (required: met.no asks for it) -email = "you@example.com" +# Your own, real email address, sent only to met.no: their terms of service ask every program for a +# way to contact its user (required) +# email = "" # Degrees Celsius or Fahrenheit. One of: "C", "F" # temperature = "C" # Rain in millimetres or inches. One of: "mm", "inch" @@ -120,8 +122,9 @@ lat = -22.91 lon = -43.17 # Name shown on the screen, e.g. Berlin (else lat, lon) place = "Rio" -# Your email address, sent only to met.no (required: met.no asks for it) -email = "you@example.com" +# Your own, real email address, sent only to met.no: their terms of service ask every program for a +# way to contact its user (required) +# email = "" # Degrees Celsius or Fahrenheit. One of: "C", "F" # temperature = "C" # Rain in millimetres or inches. One of: "mm", "inch" diff --git a/src/paperpi/cli.py b/src/paperpi/cli.py index eea5693..b8b18ee 100644 --- a/src/paperpi/cli.py +++ b/src/paperpi/cli.py @@ -324,7 +324,7 @@ def load() -> config.Config: print(f"web interface on port {web.port}") try: with screen: - count = sum(1 for p in loaded.plugins if p.entry.enabled and p.plugin.type != "default") + count = sum(1 for p in loaded.plugins if p.shown and p.plugin.type != "default") where = f"images in {out}" if display.type == "virtual" else f"screen {display.type}" print( f"showing {count} plugin{'' if count == 1 else 's'}; {where}; " @@ -389,7 +389,7 @@ def _list(args: argparse.Namespace) -> int: ( row.name, row.type, - "yes" if row.enabled else "no", + _on(row), row.level, f"{row.display_time:g} s", f"{row.refresh:g} s", @@ -411,6 +411,13 @@ def _list(args: argparse.Namespace) -> int: return 0 +def _on(row: config.PluginRow) -> str: + """The "on" column of ``paperpi list``.""" + if not row.enabled: + return "no" + return f"needs {', '.join(row.missing)}" if row.missing else "yes" + + def _example_config(args: argparse.Namespace) -> int: text = example.example_config() if args.output is None: diff --git a/src/paperpi/config.py b/src/paperpi/config.py index d898760..35af6fb 100644 --- a/src/paperpi/config.py +++ b/src/paperpi/config.py @@ -282,6 +282,17 @@ def folder_name(self) -> str: """The name of this plugin's storage folder, made from its name.""" return folder_name(self.entry.name) + @property + def missing(self) -> tuple[str, ...]: + """Required settings that are not set yet (see :func:`paperpi.plugin.setting`). + Until they are, the plugin is not shown.""" + return self.plugin.missing(self.settings) + + @property + def shown(self) -> bool: + """Switched on, and every required setting is set.""" + return self.entry.enabled and not self.missing + @dataclass class Config: @@ -324,6 +335,8 @@ class PluginRow: """As used: the setting, or the plugin's suggestion.""" storage_days: int """As used: the setting, or the plugin's suggestion (0 = files are kept).""" + missing: tuple[str, ...] = () + """Required settings that are not set yet; until they are, the plugin is not shown.""" def plugin_rows(config: Config) -> list[PluginRow]: @@ -341,6 +354,7 @@ def plugin_rows(config: Config) -> list[PluginRow]: layout=p.layout, storage_mb=p.storage_mb, storage_days=p.storage_days, + missing=p.missing, ) for p in config.plugins ] @@ -740,6 +754,15 @@ def failed() -> bool: return None checked = PluginConfig(entry, settings, plugin, self.lines.get(section)) + if entry.enabled and checked.missing: + names = ", ".join(checked.missing) + self.add( + "warning", + f"not shown until these required settings are filled in: {names}", + section, + next((k for k in checked.missing if k in block), None), + where, + ) if entry.level == "rotation" and checked.refresh == entry.display_time: key = "refresh" if "refresh" in block else "display_time" self.add( diff --git a/src/paperpi/example.py b/src/paperpi/example.py index 07dae93..4b7ad26 100644 --- a/src/paperpi/example.py +++ b/src/paperpi/example.py @@ -32,7 +32,7 @@ DisplaySettings, WebSettings, ) -from .plugin import Plugin, PluginEntry +from .plugin import Plugin, PluginEntry, is_required #: The blocks of the example file: name, plugin type and the settings that are set. #: Weather twice, to show that one plugin type can be used more than once. @@ -41,12 +41,12 @@ ( "Weather Berlin", "met_no", - {"lat": 52.52, "lon": 13.40, "place": "Berlin", "email": "you@example.com"}, + {"lat": 52.52, "lon": 13.40, "place": "Berlin"}, ), ( "Weather Rio", "met_no", - {"lat": -22.91, "lon": -43.17, "place": "Rio", "email": "you@example.com"}, + {"lat": -22.91, "lon": -43.17, "place": "Rio"}, ), ) @@ -57,8 +57,9 @@ # The file only needs the settings that differ from the default. Every other setting is # shown as a comment with its default: remove the # in front of it to change it. # Each [[plugin]] block is one plugin on the screen; the same plugin type can be used -# more than once, with a different name. The weather blocks need your own email address -# in "email": met.no asks every program for a way to contact its user. +# more than once, with a different name. The weather blocks are not shown until you fill +# in your own, real email address in "email": met.no asks every program for a way to +# contact its user. Settings marked "(required)" must be filled in before a plugin is shown. """ #: Settings whose default is "empty", but that mean a known value; shown with that value. @@ -181,6 +182,8 @@ def _settings( def _setting(key: str, info: FieldInfo, value: Any, *, comment: bool = False) -> list[str]: """The help line and the ``key = value`` line of one setting.""" help_text = info.description or key + if is_required(info): + help_text += " (required)" choices = _choices(info.annotation) named = all(re.search(rf"\b{re.escape(str(c))}\b", help_text) for c in choices) if choices and not named: diff --git a/src/paperpi/plugin.py b/src/paperpi/plugin.py index 11196ea..f8e2210 100644 --- a/src/paperpi/plugin.py +++ b/src/paperpi/plugin.py @@ -29,6 +29,7 @@ from epdlib import Layout, ScreenMode from PIL import Image from pydantic import BaseModel, ConfigDict, Field, field_validator +from pydantic.fields import FieldInfo from . import limits @@ -90,12 +91,47 @@ class Settings(PluginSettings): The same description checks the config file, builds the web interface's forms (M5) and the docs. A plugin may not use the names of the shared settings - (:data:`SHARED_SETTINGS`) for its own. + (:data:`SHARED_SETTINGS`) for its own. Use :func:`setting` instead of ``Field`` for a + setting PaperPi must know more about, such as one the user has to fill in. """ model_config = ConfigDict(extra="ignore", frozen=True) +#: The key under which :func:`setting` keeps PaperPi's own options in a pydantic field. +_OPTIONS = "paperpi" + + +def setting(default: Any = None, *, required: bool = False, **field: Any) -> Any: + """A plugin setting: pydantic's ``Field`` (with the same ``description``, ``ge``, + ``max_length``, ...) plus what PaperPi needs to know about it. This is the one place a + setting asks PaperPi for help; later options (such as a helper that looks up a place's + latitude and longitude in the web interface) are added here too:: + + lat: float | None = setting(None, required=True, description="Latitude") + + ``required``: the plugin can't work until the user fills it in (a place, an email + address, an API key). Its default must be "not set": ``None`` or ``""``. A plugin + with a required setting that is not set is not shown; the config check and the web + interface say which settings it needs. + """ + options = {"required": True} if required else {} + return Field(default, json_schema_extra={_OPTIONS: options} if options else None, **field) + + +def is_required(info: FieldInfo) -> bool: + """True for a setting made with ``setting(required=True)``.""" + extra = info.json_schema_extra + return isinstance(extra, dict) and bool(extra.get(_OPTIONS, {}).get("required")) + + +def is_set(value: Any) -> bool: + """False for "not set": ``None``, ``""`` or an empty secret.""" + if hasattr(value, "get_secret_value"): + value = value.get_secret_value() + return value is not None and value != "" + + class PluginEntry(BaseModel): """The settings every ``[[plugin]]`` block in the config file has. @@ -260,9 +296,12 @@ def __post_init__(self) -> None: if clash: problems.append(f"settings use the names of shared settings: {', '.join(clash)}") try: - self.settings() + defaults = self.settings() except ValueError: problems.append("every setting needs a default") + else: + if self.missing(defaults) != self.required: + problems.append('a required setting\'s default must be "not set" (None or "")') if not self.layouts: problems.append("needs at least one layout") if not limits.SHORTEST_REFRESH <= self.refresh <= limits.LONGEST_SETTING: @@ -281,6 +320,15 @@ def __post_init__(self) -> None: def default_layout(self) -> str: return next(iter(self.layouts)) + @property + def required(self) -> tuple[str, ...]: + """The settings the user has to fill in (see :func:`setting`).""" + return tuple(k for k, info in self.settings.model_fields.items() if is_required(info)) + + def missing(self, settings: PluginSettings) -> tuple[str, ...]: + """The required settings that are not set in ``settings``.""" + return tuple(k for k in self.required if not is_set(getattr(settings, k))) + def layout( self, name: str, settings: PluginSettings, colors: tuple[str, str] | None = None ) -> Layout: diff --git a/src/paperpi/plugins/met_no/README.md b/src/paperpi/plugins/met_no/README.md index 96dc301..1061905 100644 --- a/src/paperpi/plugins/met_no/README.md +++ b/src/paperpi/plugins/met_no/README.md @@ -50,7 +50,7 @@ Every layout shows the place: the `place` setting, or the coordinates when it is |---|---|---| | `lat` | none, required | latitude of the place, e.g. `52.52` | | `lon` | none, required | longitude of the place, e.g. `13.40` | -| `email` | none, required | your email address, sent only to met.no. met.no requires contact details from every program, so it can ask before blocking one that misbehaves | +| `email` | none, required | your own, real email address, sent only to met.no. met.no's terms of service require contact details from every program, so it can ask before blocking one that misbehaves | | `place` | `""` | name shown at the top, e.g. `"Berlin"`. Without it, the coordinates are shown ("52.52, 13.40") | | `temperature` | `"C"` | `"C"` (Celsius) or `"F"` (Fahrenheit) | | `rain` | `"mm"` | `"mm"` or `"inch"` | @@ -68,11 +68,11 @@ type = "met_no" lat = 52.52 lon = 13.40 place = "Berlin" -email = "you@example.com" +email = "you@example.com" # put your own, real address here layout = "steps_3h" # optional; without it: hours_12 ``` -Try it without a screen: `uv run paperpi render met_no --set place=Berlin` (sample data), or with real data: `uv run paperpi render met_no --live --set lat=52.52 --set lon=13.40 --set email=you@example.com`. +Try it without a screen: `uv run paperpi render met_no --set place=Berlin` (sample data), or with real data: `uv run paperpi render met_no --live --set lat=52.52 --set lon=13.40 --set email=`. Without lat, lon and email the plugin is not shown, and the config check says which of them is missing. The screen shows "Data: MET Norway" next to the "Updated" time, as met.no's data licence asks. The weather data is from [MET Norway](https://www.met.no/en) (the Norwegian Meteorological Institute), under the [Creative Commons 4.0 BY International](https://creativecommons.org/licenses/by/4.0/) licence. The weather icons are met.no's own, from [github.com/metno/weathericons](https://github.com/metno/weathericons), under the MIT licence ([`icons/LICENSE`](icons/LICENSE)). diff --git a/src/paperpi/plugins/met_no/__init__.py b/src/paperpi/plugins/met_no/__init__.py index 3b61e71..76cf1d5 100644 --- a/src/paperpi/plugins/met_no/__init__.py +++ b/src/paperpi/plugins/met_no/__init__.py @@ -16,7 +16,7 @@ from ... import webrequest from ...files import write_atomic -from ...plugin import Context, Plugin, PluginSettings, ready +from ...plugin import Context, Plugin, PluginSettings, ready, setting from . import barbs, forecast from .forecast import Forecast, Hour from .layouts import HOURS, LAYOUTS @@ -35,20 +35,22 @@ class Settings(PluginSettings): - lat: float | None = Field( - None, ge=-90, le=90, description="Latitude of the place, e.g. 52.52 (required)" + lat: float | None = setting( + None, required=True, ge=-90, le=90, description="Latitude of the place, e.g. 52.52" ) - lon: float | None = Field( - None, ge=-180, le=180, description="Longitude of the place, e.g. 13.40 (required)" + lon: float | None = setting( + None, required=True, ge=-180, le=180, description="Longitude of the place, e.g. 13.40" ) place: str = Field( "", max_length=60, description="Name shown on the screen, e.g. Berlin (else lat, lon)" ) - email: str = Field( + email: str = setting( "", + required=True, max_length=200, pattern=r"^$|^[^@\s]+@[^@\s]+$", - description="Your email address, sent only to met.no (required: met.no asks for it)", + description="Your own, real email address, sent only to met.no: their terms of " + "service ask every program for a way to contact its user", ) temperature: Literal["C", "F"] = Field("C", description="Degrees Celsius or Fahrenheit") rain: Literal["mm", "inch"] = Field("mm", description="Rain in millimetres or inches") diff --git a/src/paperpi/plugins/moon_phase/README.md b/src/paperpi/plugins/moon_phase/README.md index c40daac..d7dad0e 100644 --- a/src/paperpi/plugins/moon_phase/README.md +++ b/src/paperpi/plugins/moon_phase/README.md @@ -29,7 +29,7 @@ met.no gives one answer per place and day. The plugin saves it in its storage fo |---|---|---| | `lat` | none, required | latitude of the place, e.g. `52.52` | | `lon` | none, required | longitude of the place, e.g. `13.40` | -| `email` | none, required | your email address, sent only to met.no. met.no requires contact details from every program, so it can ask before blocking one that misbehaves | +| `email` | none, required | your own, real email address, sent only to met.no. met.no's terms of service require contact details from every program, so it can ask before blocking one that misbehaves | It suggests a refresh every 20 minutes. @@ -41,11 +41,11 @@ name = "Moon" type = "moon_phase" lat = 52.52 lon = 13.40 -email = "you@example.com" +email = "you@example.com" # put your own, real address here layout = "moon_only" # optional; without it: moon_data ``` -Try it without a screen: `uv run paperpi render moon_phase` (sample data), or with real data: `uv run paperpi render moon_phase --live --set lat=52.52 --set lon=13.40 --set email=you@example.com`. +Try it without a screen: `uv run paperpi render moon_phase` (sample data), or with real data: `uv run paperpi render moon_phase --live --set lat=52.52 --set lon=13.40 --set email=`. Without lat, lon and email the plugin is not shown, and the config check says which of them is missing. The moon data is from [MET Norway](https://www.met.no/en), under the [Creative Commons Attribution 4.0 International](https://creativecommons.org/licenses/by/4.0/) licence (CC BY 4.0): anyone may use the data, as long as they say where it came from. The `moon_data` layout shows "Data: MET Norway", as the licence asks. The moon pictures are by [NASA's Scientific Visualization Studio](https://svs.gsfc.nasa.gov/4955) (credit: NASA's Scientific Visualization Studio). NASA's pictures are generally not protected by copyright in the United States and may be used freely, as long as NASA is credited and the use does not suggest that NASA endorses PaperPi ([NASA's media usage guidelines](https://www.nasa.gov/nasa-brand-center/images-and-media/)). They are the same files as in PaperPi v1. The font is Anton, under the SIL Open Font License ([`fonts/Anton-OFL.txt`](../../fonts/Anton-OFL.txt)), in PaperPi's shared fonts folder. diff --git a/src/paperpi/plugins/moon_phase/__init__.py b/src/paperpi/plugins/moon_phase/__init__.py index f19bc63..0542a42 100644 --- a/src/paperpi/plugins/moon_phase/__init__.py +++ b/src/paperpi/plugins/moon_phase/__init__.py @@ -10,11 +10,10 @@ from zoneinfo import ZoneInfo from PIL import Image -from pydantic import Field from ... import webrequest from ...files import write_atomic -from ...plugin import Context, Plugin, PluginSettings, ready +from ...plugin import Context, Plugin, PluginSettings, ready, setting from .layouts import LAYOUTS log = logging.getLogger(__name__) @@ -42,17 +41,19 @@ class Settings(PluginSettings): - lat: float | None = Field( - None, ge=-90, le=90, description="Latitude of the place, e.g. 52.52 (required)" + lat: float | None = setting( + None, required=True, ge=-90, le=90, description="Latitude of the place, e.g. 52.52" ) - lon: float | None = Field( - None, ge=-180, le=180, description="Longitude of the place, e.g. 13.40 (required)" + lon: float | None = setting( + None, required=True, ge=-180, le=180, description="Longitude of the place, e.g. 13.40" ) - email: str = Field( + email: str = setting( "", + required=True, max_length=200, pattern=r"^$|^[^@\s]+@[^@\s]+$", - description="Your email address, sent only to met.no (required: met.no asks for it)", + description="Your own, real email address, sent only to met.no: their terms of " + "service ask every program for a way to contact its user", ) diff --git a/src/paperpi/scheduler.py b/src/paperpi/scheduler.py index 2c06712..02e3017 100644 --- a/src/paperpi/scheduler.py +++ b/src/paperpi/scheduler.py @@ -362,7 +362,7 @@ def _apply(self, config: Config) -> None: old = {slot.name: slot for slot in self._slots} slots = [] for found in config.plugins: - if not found.entry.enabled or found.plugin.type == "default": + if not found.shown or found.plugin.type == "default": continue slot = old.get(found.entry.name) if slot is None or redraw or not _same_plugin(slot.config, found): diff --git a/tests/test_cli.py b/tests/test_cli.py index 38419ca..be8180a 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -461,6 +461,8 @@ def test_list_shows_the_plugins_as_used(capsys): assert lines[0].split() == header assert lines[1].split() == "Clock basic_clock yes rotation 120 s 60 s time 500 MB, 30 d".split() assert lines[3].startswith("Weather Rio ") + # Not shown until the user fills in their email address. + assert lines[2].split()[:5] == ["Weather", "Berlin", "met_no", "needs", "email"] def test_list_says_how_many_problems(tmp_path, capsys): diff --git a/tests/test_config.py b/tests/test_config.py index 8e71bec..c9cae70 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -679,3 +679,49 @@ def test_hint_when_the_fallback_clock_is_switched_off(): assert len(cfg.plugins) == 1 [hint] = problems(cfg, "hint") assert "fallback_clock = false is not recommended" in hint + + +WEATHER = """\ +config_version = 1 +[display] +type = "virtual" +[[plugin]] +name = "Weather" +type = "met_no" +lat = 52.52 +lon = 13.40 +""" + + +def test_plugin_without_its_required_settings_is_not_shown(): + loaded = parse(WEATHER) + weather = loaded.plugin("Weather") + assert weather.missing == ("email",) + assert weather.entry.enabled and not weather.shown + assert [(p.level, p.message, p.line) for p in loaded.problems] == [ + ("warning", "not shown until these required settings are filled in: email", 4) + ] + assert config.plugin_rows(loaded)[0].missing == ("email",) + + +def test_required_setting_warning_points_at_the_setting_when_it_is_there(): + loaded = parse(WEATHER.replace("lat = 52.52\nlon = 13.40\n", 'lat = 52.52\nemail = ""\n')) + assert loaded.plugin("Weather").missing == ("lon", "email") + assert [(p.message, p.line) for p in loaded.problems] == [ + ("not shown until these required settings are filled in: lon, email", 8) + ] + loaded = parse(WEATHER.replace("lat = 52.52\nlon = 13.40\n", 'lon = 13.40\nlat = ""\n')) + assert [(p.level, p.line) for p in loaded.problems] == [("error", 8)] # "" is no number + + +def test_plugin_with_its_required_settings_is_shown(): + loaded = parse(WEATHER + 'email = "me@example.com"\n') + assert loaded.problems == [] + assert loaded.plugin("Weather").shown + + +def test_switched_off_plugin_without_required_settings_gives_no_warning(): + loaded = parse(WEATHER + "enabled = false\n") + assert loaded.problems == [] + weather = loaded.plugin("Weather") + assert weather.missing == ("email",) and not weather.shown diff --git a/tests/test_example.py b/tests/test_example.py index b31014d..860a532 100644 --- a/tests/test_example.py +++ b/tests/test_example.py @@ -25,7 +25,12 @@ def test_example_file_is_up_to_date(): def test_example_file_works_as_it_is(): loaded = config.parse(EXAMPLE_FILE.read_text()) - assert loaded.problems == [] + # The weather blocks wait for the user's own email address (met.no's terms). + assert [(p.level, p.message, p.where) for p in loaded.problems] == [ + ("warning", "not shown until these required settings are filled in: email", where) + for where in ("[[plugin]] 'Weather Berlin'", "[[plugin]] 'Weather Rio'") + ] + assert [p.entry.name for p in loaded.plugins if p.shown] == ["Clock"] assert [(p.entry.name, p.plugin.type) for p in loaded.plugins] == [ ("Clock", "basic_clock"), ("Weather Berlin", "met_no"), @@ -46,7 +51,10 @@ def test_every_default_shown_is_a_valid_setting(plugin_type): plugin = plugins.load(plugin_type) block = plugin_block(plugin, "Test", shared=True) loaded = config.parse(HEAD + uncomment(block)) - assert [p.level for p in loaded.problems if p.level != "hint"] == [] + waits = [f"not shown until these required settings are filled in: {', '.join(plugin.required)}"] + assert [p.message for p in loaded.problems if p.level != "hint"] == ( + waits if plugin.required else [] + ) found = loaded.plugin("Test") assert found.settings == plugin.settings() assert found.refresh == plugin.refresh @@ -60,7 +68,8 @@ def test_every_default_shown_is_a_valid_setting(plugin_type): def test_every_default_in_the_example_is_valid(): # Also the [display] part. loaded = config.parse(uncomment(example_config())) - assert [p for p in loaded.problems if p.level != "hint"] == [] + waiting = "not shown until these required settings are filled in: email" + assert [p.message for p in loaded.problems if p.level != "hint"] == [waiting, waiting] assert loaded.display.size == config.DisplaySettings(type="virtual").size @@ -182,3 +191,10 @@ def test_plugin_rows(): config.PluginRow("Clock", "basic_clock", True, "rotation", 120, 60, "time", 500, 30), config.PluginRow("Big", "basic_clock", False, "interrupt", 90, 300, "time_date", 20000, 0), ] + + +def test_block_marks_the_required_settings(): + block = plugin_block(plugins.load("met_no"), "Weather", {"lat": 1, "lon": 2}) + assert "# Latitude of the place, e.g. 52.52 (required)\nlat = 1\n" in block + assert 'way to contact its user (required)\n# email = ""\n' in block + assert "Berlin (else lat, lon)\n# place" in block # not required diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 11e53cb..fca04b3 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -2,7 +2,7 @@ import pytest from epdlib import ScreenMode -from pydantic import Field +from pydantic import Field, SecretStr from paperpi import plugins from paperpi.plugin import ( @@ -14,7 +14,10 @@ PluginSettings, State, draw_update, + is_required, + is_set, ready, + setting, ) @@ -59,6 +62,10 @@ class NotSettings: pass +class RequiredWithValue(PluginSettings): + email: str = setting("me@example.com", required=True) + + @pytest.mark.parametrize( ("changes", "message"), [ @@ -66,6 +73,7 @@ class NotSettings: ({"settings": NotSettings}, "subclass of PluginSettings"), ({"settings": Clash}, "names of shared settings: name, refresh"), ({"settings": NoDefault}, "every setting needs a default"), + ({"settings": RequiredWithValue}, 'a required setting\'s default must be "not set"'), ({"layouts": {}}, "at least one layout"), ({"refresh": 0}, "refresh must be between 5 and 604800 seconds"), ({"refresh": 4.9}, "refresh must be between 5 and 604800 seconds"), @@ -177,3 +185,44 @@ def test_loader_unknown_type(): def test_loader_broken_plugins(plugin_type, message): with pytest.raises(PluginDefinitionError, match=message): plugins.load(plugin_type, "tests.fake_plugins") + + +class Needs(PluginSettings): + place: str = setting("", required=True, max_length=10, description="Where") + lat: float | None = setting(None, required=True, ge=-90, le=90) + key: SecretStr = setting(SecretStr(""), required=True) + word: str = setting("hi") + + +def test_setting_is_a_field_with_paperpis_options(): + fields = Needs.model_fields + assert [k for k, info in fields.items() if is_required(info)] == ["place", "lat", "key"] + assert not is_required(Settings.model_fields["word"]) + assert fields["place"].description == "Where" + with pytest.raises(ValueError, match="at most 10 characters"): + Needs(place="far too long a place") + with pytest.raises(ValueError, match="less than or equal to 90"): + Needs(lat=91) + + +@pytest.mark.parametrize( + ("value", "expected"), + [(None, False), ("", False), (SecretStr(""), False), ("x", True), (0, True), (0.0, True), + (SecretStr("k"), True), (False, True)], +) # fmt: skip +def test_is_set(value, expected): + assert is_set(value) is expected + + +def test_missing_lists_the_required_settings_that_are_not_set(): + plugin = make(settings=Needs) + assert plugin.required == ("place", "lat", "key") + assert plugin.missing(Needs()) == ("place", "lat", "key") + assert plugin.missing(Needs(place="Rio", lat=0, key=SecretStr("k"))) == () + assert make().required == () + + +@pytest.mark.parametrize("plugin_type", ["met_no", "moon_phase"]) +def test_met_no_plugins_need_a_place_and_an_email_address(plugin_type): + # met.no's terms of service ask for a real way to contact the user. + assert plugins.load(plugin_type).required == ("lat", "lon", "email") diff --git a/tests/test_scheduler.py b/tests/test_scheduler.py index a8cf1e6..92243a4 100644 --- a/tests/test_scheduler.py +++ b/tests/test_scheduler.py @@ -540,6 +540,20 @@ def test_default_says_when_no_plugin_is_switched_on(tmp_path): assert sim.shown == ["default 0/0"] +def test_plugin_without_its_required_settings_is_not_updated(tmp_path): + weather = rotation("w") + '\ntype = "met_no"\nlat = 1\nlon = 2' + sim = Sim(tmp_path, rotation("a"), weather) + sim.plan(a="A", w="W") + sim.run(until=100) + assert sim.updates.times("w") == [] + assert sim.shown == ["A"] + # Filling in the email address (a reload) starts it. + sim.next_config = make_config(rotation("a"), weather + '\nemail = "me@example.com"') + sim.at(100, sim.scheduler.reload) + sim.run(until=150) + assert sim.updates.times("w")[0] == 101 + + def clock_labels(t): """A plan for the fallback clock: a new picture every minute.""" return f"clock {int(t // 60)}" From 023705e4d5935a3c244fd954ad83c792ea36be6b Mon Sep 17 00:00:00 2001 From: txoof-bot <337660340+txoof-bot@users.noreply.github.com> Date: Wed, 7 Oct 2026 15:54:39 +0200 Subject: [PATCH 2/4] Review fixes: setting() options, screen message, docs, more tests Co-Authored-By: Claude Opus 5.5 --- README.md | 4 +-- docs/decisions/config-format.md | 3 ++ docs/decisions/live-config-reload.md | 1 + docs/decisions/plugin-interface.md | 2 +- docs/writing-plugins.md | 4 +-- paperpi.example.toml | 13 ++++---- src/paperpi/cli.py | 12 ++++++-- src/paperpi/example.py | 7 +++-- src/paperpi/plugin.py | 36 ++++++++++++++-------- src/paperpi/plugins/default/README.md | 2 +- src/paperpi/plugins/default/__init__.py | 5 +-- src/paperpi/plugins/met_no/README.md | 2 +- src/paperpi/plugins/met_no/__init__.py | 4 +-- src/paperpi/plugins/moon_phase/README.md | 2 +- src/paperpi/plugins/moon_phase/__init__.py | 4 +-- src/paperpi/scheduler.py | 5 +-- tests/test_cli.py | 33 ++++++++++++++++++++ tests/test_config.py | 1 + tests/test_debugging.py | 2 +- tests/test_example.py | 2 +- tests/test_plugin.py | 29 +++++++++++++++-- tests/test_scheduler.py | 18 +++++++++++ 22 files changed, 144 insertions(+), 47 deletions(-) diff --git a/README.md b/README.md index 4bfbf5a..3b7ef4e 100644 --- a/README.md +++ b/README.md @@ -68,14 +68,14 @@ Run `uv run paperpi render --help` for all options. How to write a plugin: [docs ### A config file -`paperpi example-config` prints an example config file that works as it is: a virtual screen, a clock, and the weather in Berlin and Rio. Every other setting is a comment with its default and a short help text. [paperpi.example.toml](paperpi.example.toml) is the same text. The weather blocks are not shown until you put your own, real email address in `email`: met.no's terms of service ask for it, so the example leaves it empty. `paperpi list` shows them as "needs email" until then. `-o` doesn't replace a file that is already there, unless you add `--force`. +`paperpi example-config` prints an example config file that loads without errors: a virtual screen, a clock, and weather blocks for Berlin and Rio. Every other setting is a comment with its default and a short help text. [paperpi.example.toml](paperpi.example.toml) is the same text. The weather blocks are not shown until you remove the `#` in front of `email` and fill in your own, real email address: met.no's terms of service ask for it, so the example leaves it empty. Until then `paperpi list` shows them as "needs email". `-o` doesn't replace a file that is already there, unless you add `--force`. ```bash uv run paperpi example-config -o paperpi.toml uv run paperpi list --config paperpi.toml ``` -`paperpi list` shows the plugins of a config file, one line each, in the order of the file: name, type, on or off, level, display time, refresh, layout and storage (the ones used: the setting, or else the plugin's suggestion or first layout; storage is the size limit and the age limit, for example `500 MB, 30 d`, or `500 MB, no age limit`). Anything wrong with the file is shown first; a block with an error is left out of the list. +`paperpi list` shows the plugins of a config file, one line each, in the order of the file: name, type, on (`yes`, `no`, or `needs ` when a required setting is missing), level, display time, refresh, layout and storage (the ones used: the setting, or else the plugin's suggestion or first layout; storage is the size limit and the age limit, for example `500 MB, 30 d`, or `500 MB, no age limit`). Anything wrong with the file is shown first; a block with an error is left out of the list. ``` name type on level display refresh layout diff --git a/docs/decisions/config-format.md b/docs/decisions/config-format.md index a097d56..684602b 100644 --- a/docs/decisions/config-format.md +++ b/docs/decisions/config-format.md @@ -66,6 +66,8 @@ display_time = 255 The file only holds settings that differ from the default. This keeps it short and easy to fix by hand. When a later version improves a default, you get it automatically. The web interface shows every setting with its default filled in. `paperpi.example.toml` is generated from the program (`paperpi example-config`), so it is always up to date; a test checks the copy in the repository. It is short and works as it is: `[display]` with a virtual screen, a clock, and the weather in Berlin and in Rio (the same plugin type twice). Every setting it doesn't set is a comment with its default, and its help text on the line above. The first `[[plugin]]` block lists all the settings every plugin has; the other blocks list only `refresh` and `layout`, because their defaults depend on the plugin. `storage_mb` and `storage_days` can also differ per plugin, but most plugins keep the defaults, so they are only in the first block; `paperpi list` shows the values used (M4 issue #224). The copy in the repository is not edited by hand: it is made again with `paperpi example-config -o paperpi.example.toml --force`. Agreed with txoof on 2026-10-06: the example does not list every plugin. Adding and removing plugin blocks is the job of a config manager (the web interface, M5), which builds a block the same way (`paperpi.example.plugin_block`). That function refuses unknown settings and values the plugin would not accept, and checks that PaperPi reads the block back as written. Plugin names may not hold control characters (such as tab, new line or the terminal's ESC), because they can't be written back to the file reliably. +*Update 2026-10-07 (M5 part 2a, issue #238):* the example no longer fills in a made-up email address, because met.no's terms of service ask for a real one. It loads without errors, but the weather blocks are not shown, and the check warns, until you fill in `email`. + ### Checking the file Each part of the config (display, web, each plugin type) is described in code as a list of settings, each with a type, a default and a short help text. The `pydantic` package does this. From the same description we get: @@ -82,6 +84,7 @@ What happens when something is wrong: | File can't be read at all, or the `display` / `web` part is wrong | Runs on the **last good** copy, a copy that is saved each time the config loads correctly. A warning on the screen and in the web interface names the wrong line. | | ...and there is no last good copy (first install) | Shows an error screen with a QR code (a square barcode a phone camera can scan) that opens the web interface. | | One plugin block is wrong (missing value, text where a number belongs) | Only that plugin is switched off. Everything else runs. The web interface shows which setting is wrong. | +| A switched-on plugin is missing a required setting (e.g. `email`, see `plugin-interface.md`) | The plugin is not shown and not counted as broken. A warning names the missing settings; `paperpi list` shows "needs …". | | Unknown setting name, e.g. `lattitude` | Warning only: "unknown setting, did you mean `latitude`?" | The web interface only accepts valid values, so most of these mistakes can only happen through hand edits. diff --git a/docs/decisions/live-config-reload.md b/docs/decisions/live-config-reload.md index fbd42c4..6472880 100644 --- a/docs/decisions/live-config-reload.md +++ b/docs/decisions/live-config-reload.md @@ -38,6 +38,7 @@ What is on screen only lasts until the next cycle anyway, so this is kept simple - A changed plugin that is on screen is updated and redrawn right away, so the user sees the result. - A plugin that is removed or switched off while on screen: rotation moves on to the next plugin. - A new plugin joins the end of the rotation. +- *(M5 part 2a)* A plugin that is missing a required setting is treated as switched off: filling the setting in and reloading starts it (at its place in the file), and emptying it takes the plugin out of the rotation. **Update 2026-10-05 (M4, issue #205):** plugins take turns in the order of the config file, so a new plugin takes the place where it is in the file (the end, when it is added at the end). Until reloading screen settings is built (M4 issue #222, part 5b), changed screen settings take effect at the next start, with a warning in the log. *(Built in part 5b; see the table below.)* diff --git a/docs/decisions/plugin-interface.md b/docs/decisions/plugin-interface.md index 0ba9457..fb87614 100644 --- a/docs/decisions/plugin-interface.md +++ b/docs/decisions/plugin-interface.md @@ -50,7 +50,7 @@ The plugin never talks to the screen. Only the scheduler does. For sample images and tests, `fetch` is skipped and the sample data goes straight to `draw`. How to write a plugin: `docs/writing-plugins.md`. -**Update 2026-10-07 (M5 part 2a, issue #238, agreed with txoof):** a plugin can mark a setting as **required** with `setting(..., required=True)`, for settings it can't work without, such as an API key. The first ones are `lat`, `lon` and `email` of `met_no` and `moon_phase`: met.no's terms of service ask for a real email address, and a user should not start these plugins without giving one. A plugin that is missing a required setting can be in the config file and switched on, but it is **not shown and not counted as broken**: the config check gives a warning naming the missing settings, `paperpi list` shows "needs email" in its "on" column, and the web interface (part 2b) shows "Needs settings" in place of the on switch. The example config no longer fills in a made-up email address. `setting` is the one standard place where a setting asks PaperPi for help; the input helpers of `web-interface.md` (such as `location`, which looks up latitude and longitude; part 3c) will be options of `setting` too. +**Update 2026-10-07 (M5 part 2a, issue #238, agreed with txoof):** a plugin can mark a setting as **required** with `setting(..., required=True)`, for settings it can't work without, such as an API key. The first ones are `lat`, `lon` and `email` of `met_no` and `moon_phase`: met.no's terms of service ask for a real email address, and a user should not start these plugins without giving one. A plugin that is missing a required setting can be in the config file and switched on, but it is **not shown and not counted as broken**: the config check gives a warning naming the missing settings, `paperpi list` shows "needs email" in its "on" column, and the web interface (part 2b) shows "Needs settings" in place of the on switch. The example config no longer fills in a made-up email address. `setting` is the one standard place for a setting's own options; the input helpers of `web-interface.md` (such as `location`, which looks up latitude and longitude; part 3c) will be options of `setting` too. ### How a plugin is run diff --git a/docs/writing-plugins.md b/docs/writing-plugins.md index 2a0ace9..1a86ee3 100644 --- a/docs/writing-plugins.md +++ b/docs/writing-plugins.md @@ -80,7 +80,7 @@ Each setting is a field of the plugin's `Settings` class, with a type, a default - Every setting needs a default. - Don't use the names of the shared settings, which every `[[plugin]]` block already has: `name`, `type`, `enabled`, `level`, `display_time`, `refresh`, `time_limit`, `layout`, `alert_reminder`, `alert_max_time`. - Use `pydantic.SecretStr` as the type for API keys and passwords. PaperPi then never shows their values in error messages or logs. -- A setting the user must fill in before the plugin can work (a place, an email address, an API key) is made with `paperpi.plugin.setting(..., required=True)` instead of `Field`. It takes the same arguments as `Field`. Its default must be "not set": `None` or `""` (or an empty `SecretStr`). Until every required setting is filled in, the plugin is not shown and not counted as broken; the config check, `paperpi list` and the web interface say which settings are missing. The example block marks them "(required)". `setting` is the one place where a setting asks PaperPi for more help; later options, such as a helper in the web interface that looks up latitude and longitude, are added there too. +- A setting the user must fill in before the plugin can work (a place, an email address, an API key) is made with `paperpi.plugin.setting(..., required=True)` instead of `Field`. It takes the same arguments as `Field`. Its default must be "not set": `None`, empty text or an empty `SecretStr` (text of only spaces also counts as not set). Until every required setting is filled in, the plugin is not shown and not counted as broken; the config check and `paperpi list` (and from M5 part 2b the web interface) say which settings are missing. The example block marks them "(required)". `setting` is also where later options for a single setting are added, such as a web interface helper that looks up latitude and longitude (M5 part 3c). ```python from paperpi.plugin import PluginSettings, setting @@ -142,7 +142,7 @@ data = answer.json() ``` `get` takes: -- `contact`: an email or web address, added to the User-Agent header (the line in every request that names the program), so the service knows whom to ask about problems. Some services, like met.no, require it. Give your plugin a setting for it, as `met_no` does with `email`. +- `contact`: an email or web address, added to the User-Agent header (the line in every request that names the program), so the service knows whom to ask about problems. Some services, like met.no, require it. Give your plugin a setting for it, as `met_no` does with `email`, and make it required with `setting(..., required=True)`. - `headers=`: more headers to send, for example an API key. They are sent only to the server of `url`: if that server redirects to another one, they are left out. - `if_modified_since=`: the `last_modified` of an earlier answer. If nothing changed since, `answer.not_modified` is `True` and `answer.body` is empty. Save `last_modified` (it can be `None` when the server didn't send one) and the data in `context.storage`, so the next update can ask. - `max_bytes=`: a higher size limit, for example for large images. diff --git a/paperpi.example.toml b/paperpi.example.toml index b5a0c9a..62c7ebf 100644 --- a/paperpi.example.toml +++ b/paperpi.example.toml @@ -4,9 +4,10 @@ # The file only needs the settings that differ from the default. Every other setting is # shown as a comment with its default: remove the # in front of it to change it. # Each [[plugin]] block is one plugin on the screen; the same plugin type can be used -# more than once, with a different name. The weather blocks are not shown until you fill -# in your own, real email address in "email": met.no asks every program for a way to -# contact its user. Settings marked "(required)" must be filled in before a plugin is shown. +# more than once, with a different name. The weather blocks are not shown until you remove +# the # in front of "email" and fill in your own, real email address: met.no asks every +# program for a way to contact its user. Settings marked "(required)" must be filled in +# before a plugin is shown. config_version = 1 @@ -98,8 +99,7 @@ lat = 52.52 lon = 13.4 # Name shown on the screen, e.g. Berlin (else lat, lon) place = "Berlin" -# Your own, real email address, sent only to met.no: their terms of service ask every program for a -# way to contact its user (required) +# Your own, real email address, sent only to met.no (their terms of service ask for one) (required) # email = "" # Degrees Celsius or Fahrenheit. One of: "C", "F" # temperature = "C" @@ -122,8 +122,7 @@ lat = -22.91 lon = -43.17 # Name shown on the screen, e.g. Berlin (else lat, lon) place = "Rio" -# Your own, real email address, sent only to met.no: their terms of service ask every program for a -# way to contact its user (required) +# Your own, real email address, sent only to met.no (their terms of service ask for one) (required) # email = "" # Degrees Celsius or Fahrenheit. One of: "C", "F" # temperature = "C" diff --git a/src/paperpi/cli.py b/src/paperpi/cli.py index b8b18ee..678b272 100644 --- a/src/paperpi/cli.py +++ b/src/paperpi/cli.py @@ -146,7 +146,9 @@ def _parser() -> argparse.ArgumentParser: "Show the plugins of a config file, one line each, in the order of the file. " "Anything wrong with the file is shown first; a block with an error is not in the " "list. Refresh and layout are the ones used: the setting, or else the plugin's " - "suggested refresh and its first layout." + 'suggested refresh and its first layout. The "on" column says "needs ' + '" for a plugin that is switched on but not shown, because a required ' + "setting is missing." ), ) listing.add_argument( @@ -237,6 +239,10 @@ def _render(args: argparse.Namespace) -> int: if layout not in job.plugin.layouts: known = ", ".join(job.plugin.layouts) raise UsageError(f"unknown layout {layout!r}; choose from: {known}") + missing = job.plugin.missing(job.settings) + if args.live and missing: + sets = " ".join(f"--set {k}=..." for k in missing) + raise UsageError(f"{job.plugin.type} needs these required settings: {sets}") time_limit = job.time_limit if args.time_limit is None else args.time_limit if not 0 < time_limit <= limits.PLUGIN_UPDATE_MAX: raise UsageError(f"--time-limit must be above 0 and at most {limits.PLUGIN_UPDATE_MAX:g}") @@ -389,7 +395,7 @@ def _list(args: argparse.Namespace) -> int: ( row.name, row.type, - _on(row), + _on_column(row), row.level, f"{row.display_time:g} s", f"{row.refresh:g} s", @@ -411,7 +417,7 @@ def _list(args: argparse.Namespace) -> int: return 0 -def _on(row: config.PluginRow) -> str: +def _on_column(row: config.PluginRow) -> str: """The "on" column of ``paperpi list``.""" if not row.enabled: return "no" diff --git a/src/paperpi/example.py b/src/paperpi/example.py index 4b7ad26..ffc7b7f 100644 --- a/src/paperpi/example.py +++ b/src/paperpi/example.py @@ -57,9 +57,10 @@ # The file only needs the settings that differ from the default. Every other setting is # shown as a comment with its default: remove the # in front of it to change it. # Each [[plugin]] block is one plugin on the screen; the same plugin type can be used -# more than once, with a different name. The weather blocks are not shown until you fill -# in your own, real email address in "email": met.no asks every program for a way to -# contact its user. Settings marked "(required)" must be filled in before a plugin is shown. +# more than once, with a different name. The weather blocks are not shown until you remove +# the # in front of "email" and fill in your own, real email address: met.no asks every +# program for a way to contact its user. Settings marked "(required)" must be filled in +# before a plugin is shown. """ #: Settings whose default is "empty", but that mean a known value; shown with that value. diff --git a/src/paperpi/plugin.py b/src/paperpi/plugin.py index f8e2210..d17589f 100644 --- a/src/paperpi/plugin.py +++ b/src/paperpi/plugin.py @@ -102,34 +102,41 @@ class Settings(PluginSettings): _OPTIONS = "paperpi" -def setting(default: Any = None, *, required: bool = False, **field: Any) -> Any: +def setting(default: Any, *, required: bool = False, **field: Any) -> Any: """A plugin setting: pydantic's ``Field`` (with the same ``description``, ``ge``, ``max_length``, ...) plus what PaperPi needs to know about it. This is the one place a - setting asks PaperPi for help; later options (such as a helper that looks up a place's - latitude and longitude in the web interface) are added here too:: + setting gets PaperPi's own options; later ones (such as a web interface helper that + looks up a place's latitude and longitude, M5 part 3c) are added here too:: lat: float | None = setting(None, required=True, description="Latitude") ``required``: the plugin can't work until the user fills it in (a place, an email - address, an API key). Its default must be "not set": ``None`` or ``""``. A plugin - with a required setting that is not set is not shown; the config check and the web - interface say which settings it needs. + address, an API key). Its default must be "not set" (see :func:`is_set`). A plugin + with a required setting that is not set is not shown; the config check and + ``paperpi list`` (and from M5 part 2b the web interface) say which settings it needs. """ - options = {"required": True} if required else {} - return Field(default, json_schema_extra={_OPTIONS: options} if options else None, **field) + extra = field.pop("json_schema_extra", None) or {} + if not isinstance(extra, dict): + raise TypeError("setting() takes json_schema_extra only as a dictionary") + if required: + extra = extra | {_OPTIONS: {"required": True}} + return Field(default, json_schema_extra=extra or None, **field) def is_required(info: FieldInfo) -> bool: """True for a setting made with ``setting(required=True)``.""" extra = info.json_schema_extra - return isinstance(extra, dict) and bool(extra.get(_OPTIONS, {}).get("required")) + options = extra.get(_OPTIONS) if isinstance(extra, dict) else None + return isinstance(options, dict) and bool(options.get("required")) def is_set(value: Any) -> bool: - """False for "not set": ``None``, ``""`` or an empty secret.""" + """False for "not set": ``None``, empty text (also only spaces) or an empty secret.""" if hasattr(value, "get_secret_value"): value = value.get_secret_value() - return value is not None and value != "" + if isinstance(value, str | bytes): + return bool(value.strip()) + return value is not None class PluginEntry(BaseModel): @@ -227,7 +234,7 @@ class PluginsStatus: failing: int """Plugins that failed their last update, or are left out after failing again and again.""" total: int - """Plugins that are switched on.""" + """Plugins that are ready to show: switched on, with all their required settings.""" @dataclass(frozen=True) @@ -301,7 +308,10 @@ def __post_init__(self) -> None: problems.append("every setting needs a default") else: if self.missing(defaults) != self.required: - problems.append('a required setting\'s default must be "not set" (None or "")') + problems.append( + 'a required setting\'s default must be "not set" (None, "" or an empty ' + "secret)" + ) if not self.layouts: problems.append("needs at least one layout") if not limits.SHORTEST_REFRESH <= self.refresh <= limits.LONGEST_SETTING: diff --git a/src/paperpi/plugins/default/README.md b/src/paperpi/plugins/default/README.md index 0784601..29618b6 100644 --- a/src/paperpi/plugins/default/README.md +++ b/src/paperpi/plugins/default/README.md @@ -1,6 +1,6 @@ # default -Shown when nothing else can be shown because plugins are failing, or because no plugin is switched on. The scheduler tells it how many plugins are not working, and it shows e.g. "3 of 4 plugins are not working. See the web interface for more information." It needs no network. +Shown when nothing else can be shown because plugins are failing, or because no plugin is ready to show (switched on, with all its required settings filled in). The scheduler tells it how many plugins are not working, and it shows e.g. "3 of 4 plugins are not working. See the web interface for more information." It needs no network. PaperPi always has this plugin, also when the config file has no block for it. A `[[plugin]]` block with `type = "default"` changes its settings; it never takes part in the rotation. The QR code that opens the web interface comes with the web interface (M5). diff --git a/src/paperpi/plugins/default/__init__.py b/src/paperpi/plugins/default/__init__.py index e43ecbf..9ba3fee 100644 --- a/src/paperpi/plugins/default/__init__.py +++ b/src/paperpi/plugins/default/__init__.py @@ -1,4 +1,5 @@ -"""default: shown when nothing else can be: plugins are failing, or none are switched on. +"""default: shown when nothing else can be: plugins are failing, or none is ready to show +(none is switched on with all its required settings filled in). The scheduler tells it how many plugins are failing (``context.status``). The QR code that opens the web interface is added with the web interface (M5). @@ -19,7 +20,7 @@ def fetch(context: Context): def message(status: PluginsStatus) -> str: if status.total == 0: - return "No plugins are switched on." + return "No plugins are ready to show." noun = "plugin is" if status.total == 1 else "plugins are" return f"{status.failing} of {status.total} {noun} not working." diff --git a/src/paperpi/plugins/met_no/README.md b/src/paperpi/plugins/met_no/README.md index 1061905..0216931 100644 --- a/src/paperpi/plugins/met_no/README.md +++ b/src/paperpi/plugins/met_no/README.md @@ -72,7 +72,7 @@ email = "you@example.com" # put your own, real address here layout = "steps_3h" # optional; without it: hours_12 ``` -Try it without a screen: `uv run paperpi render met_no --set place=Berlin` (sample data), or with real data: `uv run paperpi render met_no --live --set lat=52.52 --set lon=13.40 --set email=`. Without lat, lon and email the plugin is not shown, and the config check says which of them is missing. +Try it without a screen: `uv run paperpi render met_no --set place=Berlin` (sample data), or with real data: `uv run paperpi render met_no --live --set lat=52.52 --set lon=13.40 --set email=you@example.com` (put your own address in place of `you@example.com`). If lat, lon or email is missing, the plugin is not shown; the config check and `paperpi list` say which one. The screen shows "Data: MET Norway" next to the "Updated" time, as met.no's data licence asks. The weather data is from [MET Norway](https://www.met.no/en) (the Norwegian Meteorological Institute), under the [Creative Commons 4.0 BY International](https://creativecommons.org/licenses/by/4.0/) licence. The weather icons are met.no's own, from [github.com/metno/weathericons](https://github.com/metno/weathericons), under the MIT licence ([`icons/LICENSE`](icons/LICENSE)). diff --git a/src/paperpi/plugins/met_no/__init__.py b/src/paperpi/plugins/met_no/__init__.py index 76cf1d5..6069cf3 100644 --- a/src/paperpi/plugins/met_no/__init__.py +++ b/src/paperpi/plugins/met_no/__init__.py @@ -49,8 +49,8 @@ class Settings(PluginSettings): required=True, max_length=200, pattern=r"^$|^[^@\s]+@[^@\s]+$", - description="Your own, real email address, sent only to met.no: their terms of " - "service ask every program for a way to contact its user", + description="Your own, real email address, sent only to met.no (their terms of " + "service ask for one)", ) temperature: Literal["C", "F"] = Field("C", description="Degrees Celsius or Fahrenheit") rain: Literal["mm", "inch"] = Field("mm", description="Rain in millimetres or inches") diff --git a/src/paperpi/plugins/moon_phase/README.md b/src/paperpi/plugins/moon_phase/README.md index d7dad0e..a7b4386 100644 --- a/src/paperpi/plugins/moon_phase/README.md +++ b/src/paperpi/plugins/moon_phase/README.md @@ -45,7 +45,7 @@ email = "you@example.com" # put your own, real address here layout = "moon_only" # optional; without it: moon_data ``` -Try it without a screen: `uv run paperpi render moon_phase` (sample data), or with real data: `uv run paperpi render moon_phase --live --set lat=52.52 --set lon=13.40 --set email=`. Without lat, lon and email the plugin is not shown, and the config check says which of them is missing. +Try it without a screen: `uv run paperpi render moon_phase` (sample data), or with real data: `uv run paperpi render moon_phase --live --set lat=52.52 --set lon=13.40 --set email=you@example.com` (put your own address in place of `you@example.com`). If lat, lon or email is missing, the plugin is not shown; the config check and `paperpi list` say which one. The moon data is from [MET Norway](https://www.met.no/en), under the [Creative Commons Attribution 4.0 International](https://creativecommons.org/licenses/by/4.0/) licence (CC BY 4.0): anyone may use the data, as long as they say where it came from. The `moon_data` layout shows "Data: MET Norway", as the licence asks. The moon pictures are by [NASA's Scientific Visualization Studio](https://svs.gsfc.nasa.gov/4955) (credit: NASA's Scientific Visualization Studio). NASA's pictures are generally not protected by copyright in the United States and may be used freely, as long as NASA is credited and the use does not suggest that NASA endorses PaperPi ([NASA's media usage guidelines](https://www.nasa.gov/nasa-brand-center/images-and-media/)). They are the same files as in PaperPi v1. The font is Anton, under the SIL Open Font License ([`fonts/Anton-OFL.txt`](../../fonts/Anton-OFL.txt)), in PaperPi's shared fonts folder. diff --git a/src/paperpi/plugins/moon_phase/__init__.py b/src/paperpi/plugins/moon_phase/__init__.py index 0542a42..065f7b9 100644 --- a/src/paperpi/plugins/moon_phase/__init__.py +++ b/src/paperpi/plugins/moon_phase/__init__.py @@ -52,8 +52,8 @@ class Settings(PluginSettings): required=True, max_length=200, pattern=r"^$|^[^@\s]+@[^@\s]+$", - description="Your own, real email address, sent only to met.no: their terms of " - "service ask every program for a way to contact its user", + description="Your own, real email address, sent only to met.no (their terms of " + "service ask for one)", ) diff --git a/src/paperpi/scheduler.py b/src/paperpi/scheduler.py index 02e3017..6f3e95f 100644 --- a/src/paperpi/scheduler.py +++ b/src/paperpi/scheduler.py @@ -40,7 +40,8 @@ - A failed update is skipped and tried again at the next refresh. After :data:`~paperpi.limits.FAILURES_BEFORE_LEFT_OUT` failures in a row the plugin is left out for :data:`~paperpi.limits.LEFT_OUT` seconds. When nothing can be shown because plugins - fail, or no plugin is switched on, the ``default`` plugin says so. When no plugin has + fail, or no plugin is ready to show (switched on, with all its required settings filled + in), the ``default`` plugin says so. When no plugin has anything to show and none fail, a small fallback clock is shown (``[display] fallback_clock``), so an empty screen is never mistaken for a broken one. @@ -639,7 +640,7 @@ def _next(self, group: list[_Slot], after: str | None) -> _Slot: def _choose_idle(self, now: float) -> _Slot | None: """No plugin has anything to show. - When plugins fail, or none are switched on: the ``default`` plugin, which says so. + When plugins fail, or none is ready to show: the ``default`` plugin, which says so. Otherwise (e.g. only a music plugin, and no music) the fallback clock, so the screen still changes every minute and can be told apart from a broken one. """ diff --git a/tests/test_cli.py b/tests/test_cli.py index be8180a..74ae37b 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -443,6 +443,39 @@ def test_run_cleans_every_plugin_folder_at_start(tmp_path, monkeypatch): assert not old.exists() +def test_run_counts_only_the_plugins_that_are_shown(tmp_path, monkeypatch, capsys): + monkeypatch.setattr(cli.Scheduler, "run", lambda self: None) + cfg = tmp_path / "paperpi.toml" + cfg.write_text( + 'config_version = 1\n[display]\ntype = "virtual"\n' + '[[plugin]]\nname = "Clock"\ntype = "basic_clock"\n' + '[[plugin]]\nname = "Weather"\ntype = "met_no"\nlat = 1\nlon = 2\n' + ) + args = ["run", "--no-web", "--config", str(cfg), "--state-dir", str(tmp_path)] + assert main([*args, "--health-file", str(tmp_path / "health")]) == 0 + assert "showing 1 plugin;" in capsys.readouterr().out + + +def test_list_says_which_required_settings_are_missing(tmp_path, capsys): + cfg = tmp_path / "paperpi.toml" + cfg.write_text( + 'config_version = 1\n[display]\ntype = "virtual"\n' + '[[plugin]]\nname = "Weather"\ntype = "met_no"\n' + '[[plugin]]\nname = "Off"\ntype = "met_no"\nenabled = false\n' + ) + assert main(["list", "--config", str(cfg)]) == 0 + lines = capsys.readouterr().out.splitlines() + assert lines[1].split()[:6] == ["Weather", "met_no", "needs", "lat,", "lon,", "email"] + assert lines[2].split()[:3] == ["Off", "met_no", "no"] # switched off: no "needs" + + +def test_render_live_says_which_required_settings_are_missing(capsys): + assert main(["render", "met_no", "--live", "--set", "lat=1"]) == 2 + assert "met_no needs these required settings: --set lon=... --set email=..." in ( + capsys.readouterr().err + ) + + def test_list_shows_no_age_limit(tmp_path, capsys): cfg = tmp_path / "paperpi.toml" cfg.write_text( diff --git a/tests/test_config.py b/tests/test_config.py index c9cae70..3d0a56d 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -710,6 +710,7 @@ def test_required_setting_warning_points_at_the_setting_when_it_is_there(): assert [(p.message, p.line) for p in loaded.problems] == [ ("not shown until these required settings are filled in: lon, email", 8) ] + # An empty value for a number is no "not set": it is an error, as before. loaded = parse(WEATHER.replace("lat = 52.52\nlon = 13.40\n", 'lon = 13.40\nlat = ""\n')) assert [(p.level, p.line) for p in loaded.problems] == [("error", 8)] # "" is no number diff --git a/tests/test_debugging.py b/tests/test_debugging.py index 6f8b32a..26f2e2e 100644 --- a/tests/test_debugging.py +++ b/tests/test_debugging.py @@ -59,7 +59,7 @@ def test_delay(tmp_path): [ (3, 4, "3 of 4 plugins are not working."), (1, 1, "1 of 1 plugin is not working."), - (0, 0, "No plugins are switched on."), + (0, 0, "No plugins are ready to show."), ], ) def test_default_message(failing, total, text): diff --git a/tests/test_example.py b/tests/test_example.py index 860a532..b2c2284 100644 --- a/tests/test_example.py +++ b/tests/test_example.py @@ -196,5 +196,5 @@ def test_plugin_rows(): def test_block_marks_the_required_settings(): block = plugin_block(plugins.load("met_no"), "Weather", {"lat": 1, "lon": 2}) assert "# Latitude of the place, e.g. 52.52 (required)\nlat = 1\n" in block - assert 'way to contact its user (required)\n# email = ""\n' in block + assert 'terms of service ask for one) (required)\n# email = ""\n' in block assert "Berlin (else lat, lon)\n# place" in block # not required diff --git a/tests/test_plugin.py b/tests/test_plugin.py index fca04b3..924fa03 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -2,7 +2,7 @@ import pytest from epdlib import ScreenMode -from pydantic import Field, SecretStr +from pydantic import Field, SecretBytes, SecretStr from paperpi import plugins from paperpi.plugin import ( @@ -197,7 +197,7 @@ class Needs(PluginSettings): def test_setting_is_a_field_with_paperpis_options(): fields = Needs.model_fields assert [k for k, info in fields.items() if is_required(info)] == ["place", "lat", "key"] - assert not is_required(Settings.model_fields["word"]) + assert not is_required(fields["word"]) assert fields["place"].description == "Where" with pytest.raises(ValueError, match="at most 10 characters"): Needs(place="far too long a place") @@ -207,13 +207,36 @@ def test_setting_is_a_field_with_paperpis_options(): @pytest.mark.parametrize( ("value", "expected"), - [(None, False), ("", False), (SecretStr(""), False), ("x", True), (0, True), (0.0, True), + [(None, False), ("", False), (" ", False), (SecretStr(""), False), (SecretStr(" "), False), + (SecretBytes(b""), False), (b"", False), ("x", True), (0, True), (0.0, True), (SecretStr("k"), True), (False, True)], ) # fmt: skip def test_is_set(value, expected): assert is_set(value) is expected +def test_setting_keeps_other_schema_extras(): + class Extra(PluginSettings): + word: str = setting("", required=True, json_schema_extra={"examples": ["hi"]}) + other: str = setting("", json_schema_extra={"paperpi": "wrong shape"}) + + info = Extra.model_fields["word"] + assert info.json_schema_extra == {"examples": ["hi"], "paperpi": {"required": True}} + assert is_required(info) + assert not is_required(Extra.model_fields["other"]) # no crash on a wrong shape + + +def test_setting_needs_a_default(): + with pytest.raises(TypeError): + setting(description="no default") + + +def test_built_in_plugins_need_no_settings(): + # PaperPi runs these with their defaults (the screen shown when nothing else can be). + for plugin_type in ("basic_clock", "default"): + assert plugins.load(plugin_type).required == () + + def test_missing_lists_the_required_settings_that_are_not_set(): plugin = make(settings=Needs) assert plugin.required == ("place", "lat", "key") diff --git a/tests/test_scheduler.py b/tests/test_scheduler.py index 92243a4..bb2cd9e 100644 --- a/tests/test_scheduler.py +++ b/tests/test_scheduler.py @@ -554,6 +554,24 @@ def test_plugin_without_its_required_settings_is_not_updated(tmp_path): assert sim.updates.times("w")[0] == 101 +def test_reload_without_a_required_setting_takes_the_plugin_off_screen(tmp_path): + weather = rotation("w") + '\ntype = "met_no"\nlat = 1\nlon = 2' + sim = Sim(tmp_path, rotation("a"), weather + '\nemail = "me@example.com"') + sim.plan(a="A", w="W") + sim.run(until=150) # w is on screen since 101 + sim.next_config = make_config(rotation("a"), weather) + sim.scheduler.reload() + sim.run(until=300) + assert sim.writes[:3] == [(1, "A"), (101, "W"), (150, "A")] + assert [t for t in sim.updates.times("w") if t >= 150] == [] + + +def test_default_says_so_when_every_plugin_waits_for_its_settings(tmp_path): + sim = Sim(tmp_path, rotation("w") + '\ntype = "met_no"\nlat = 1\nlon = 2') + sim.run(until=100) + assert sim.shown == ["default 0/0"] # "No plugins are ready to show." + + def clock_labels(t): """A plan for the fallback clock: a new picture every minute.""" return f"clock {int(t // 60)}" From d639c7ddbabd33bd8f663caac8dfccc79e505251 Mon Sep 17 00:00:00 2001 From: txoof-bot <337660340+txoof-bot@users.noreply.github.com> Date: Wed, 7 Oct 2026 18:14:32 +0200 Subject: [PATCH 3/4] Editing plugin blocks in the config file (M5 part 2b-1) Part of #238. Co-Authored-By: Claude Opus 5.5 --- docs/decisions/config-format.md | 1 + src/paperpi/web/config_file.py | 271 ++++++++++++++++++++++++++++++++ src/paperpi/web/password.py | 21 +-- tests/test_web_config_file.py | 129 +++++++++++++++ 4 files changed, 407 insertions(+), 15 deletions(-) create mode 100644 src/paperpi/web/config_file.py create mode 100644 tests/test_web_config_file.py diff --git a/docs/decisions/config-format.md b/docs/decisions/config-format.md index 684602b..f6bcc6a 100644 --- a/docs/decisions/config-format.md +++ b/docs/decisions/config-format.md @@ -92,6 +92,7 @@ The web interface only accepts valid values, so most of these mistakes can only ### Writing the file - The web interface keeps comments that were written by hand. + *Update (M5 part 2b, issue #238):* `[[plugin]]` blocks are moved, removed, switched on or off and added by changing the file's text one block at a time (`paperpi.web.config_file`), not with tomlkit: tomlkit counts the comment lines just above a `[[plugin]]` line as part of the block before it, so they would move with the wrong plugin. Here they belong to the block below them. Every change is read back and compared before it is saved; a file written in a way this code doesn't understand is not changed, and the web interface says to change it by hand. Single settings (such as the password) are still written with tomlkit. - The file is written safely: first to a temporary file, then that file replaces the old one in one step. A power cut during a save can't leave a half-written config. ### Secrets diff --git a/src/paperpi/web/config_file.py b/src/paperpi/web/config_file.py new file mode 100644 index 0000000..7f6c287 --- /dev/null +++ b/src/paperpi/web/config_file.py @@ -0,0 +1,271 @@ +"""Changing the config file from the web interface, keeping everything else in it as it is. + +- :func:`read` and :func:`write` read the file and save a changed copy that keeps the old + file's permissions (and owner, when run as root). +- The plugin changes (:func:`move`, :func:`remove`, :func:`set_enabled`, :func:`add`) work on + the file's text, one ``[[plugin]]`` block at a time. The comment lines just above a + ``[[plugin]]`` line belong to that block and move with it. +- Every change is checked before it is saved: PaperPi must read the new text back as exactly + the old settings plus the change. A file written in a way these functions don't + understand is never saved wrongly; :class:`EditError` says so instead. +- Each change names its block by place and name, so a block that was moved or removed by + hand a moment ago is not changed by mistake (:class:`ChangedMeanwhile`). +""" + +from __future__ import annotations + +import os +import re +import stat +import tomllib +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +from .. import config +from ..files import write_atomic + + +class EditError(ValueError): + """The config file can't be read, changed or saved; the message says why.""" + + +class ChangedMeanwhile(EditError): + """The block to change is not where the page showed it: the file changed meanwhile.""" + + +def read(path: Path) -> tuple[Path, str]: + """The real path of the config file (links followed) and its text.""" + # A config file that is a link to another file: change the file it points to. + path = Path(os.path.realpath(path)) + try: + return path, config.read_text(path) + except config.ConfigError as error: + raise EditError(str(error)) from None + + +def write(path: Path, text: str) -> None: + """Save ``text`` as the config file at ``path`` (a real path, from :func:`read`).""" + try: + # The new file keeps the old one's permissions and, when run with sudo, its owner, + # so the PaperPi service can still read it. + info = os.stat(path) + owner = (info.st_uid, info.st_gid) if os.geteuid() == 0 else None + write_atomic(path, text.encode("utf-8"), mode=stat.S_IMODE(info.st_mode), owner=owner) + except OSError as error: + raise EditError(f"can't save {path}: {error}") from None + + +def load(text: str) -> dict[str, Any]: + """The settings in ``text``, as PaperPi reads them.""" + try: + return tomllib.loads(text) + except (tomllib.TOMLDecodeError, RecursionError, ValueError) as error: + raise EditError(f"the config file is not valid TOML: {error}") from None + + +@dataclass(frozen=True) +class Block: + """Where one ``[[plugin]]`` block is in the file (line numbers count from 0).""" + + start: int + """The first line: the comments just above the ``[[plugin]]`` line, or that line.""" + header: int + """The ``[[plugin]]`` line.""" + own_end: int + """The line after its own settings (where a ``[plugin.x]`` part or the next part starts).""" + end: int + """The line after the block, including blank lines and ``[plugin.x]`` parts.""" + + +_HEADER = re.compile(r"\s*(\[\[?)\s*([A-Za-z0-9_.-]+)\s*\]\]?\s*(#.*)?") +_ENABLED = re.compile(r"\s*enabled\s*=") + + +def blocks(text: str) -> list[Block]: + """The ``[[plugin]]`` blocks in ``text``, in file order.""" + lines = text.splitlines() + starts: list[tuple[int, str]] = [] # (line, "plugin" / "part of plugin" / "other") + quote = None # inside a multi-line string: the quotes it started with + for number, line in enumerate(lines): + inside = quote is not None + for found in re.finditer(r'"""|\'\'\'', line): + if quote is None: + quote = found.group() + elif found.group() == quote: + quote = None + header = None if inside else _HEADER.fullmatch(line) + if header: + brackets, name = header.group(1), header.group(2) + if brackets == "[[" and name == "plugin": + starts.append((number, "plugin")) + elif name.startswith("plugin."): + starts.append((number, "part of plugin")) + else: + starts.append((number, "other")) + found: list[Block] = [] + for index, (header, kind) in enumerate(starts): + if kind != "plugin": + continue + later = starts[index + 1 :] + own_end = later[0][0] if later else len(lines) + after = next((n for n, k in later if k != "part of plugin"), len(lines)) + end = _comments_above(lines, after, header + 1) if after < len(lines) else after + lowest = max((n + 1 for n, _ in starts[:index]), default=0) + found.append(Block(_comments_above(lines, header, lowest), header, own_end, end)) + return found + + +def _comments_above(lines: list[str], line: int, lowest: int) -> int: + """The first of the comment lines just above ``line`` (or ``line`` itself).""" + while line > lowest and lines[line - 1].lstrip().startswith("#"): + line -= 1 + return line + + +def plugin_blocks(data: dict[str, Any]) -> list[Any]: + """The ``[[plugin]]`` blocks of the loaded settings (as written, also broken ones).""" + found = data.get("plugin", []) + return found if isinstance(found, list) else [] + + +def name_of(block: Any) -> str: + """The name of a loaded plugin block, or ``""`` when it has none (a broken block).""" + name = block.get("name") if isinstance(block, dict) else None + return name if isinstance(name, str) else "" + + +def move(text: str, index: int, name: str, step: int) -> str: + """``text`` with plugin block ``index`` moved one place up (``step=-1``) or down (``1``).""" + data, found = _check(text, index, name) + other = index + step + if step not in (-1, 1) or not 0 <= other < len(found): + raise EditError("this plugin can't move further") + first, second = sorted((index, other)) + a, b = found[first], found[second] + lines = _lines(text) + new = [ + *lines[: a.start], + *_spaced(lines[b.start : b.end]), + *lines[a.end : b.start], + *lines[a.start : a.end], + *lines[b.end :], + ] + expected = plugin_blocks(data) + expected[first], expected[second] = expected[second], expected[first] + return _checked("".join(new), data | {"plugin": expected}) + + +def remove(text: str, index: int, name: str) -> str: + """``text`` without plugin block ``index``.""" + data, found = _check(text, index, name) + block = found[index] + lines = _lines(text) + new = "".join([*lines[: block.start], *lines[block.end :]]) + expected = plugin_blocks(data) + del expected[index] + data = {k: v for k, v in data.items() if k != "plugin"} + return _checked(new, data | {"plugin": expected} if expected else data) + + +def set_enabled(text: str, index: int, name: str, enabled: bool) -> str: + """``text`` with plugin block ``index`` switched on or off. + + Off writes ``enabled = false``; on removes that line, because the file only holds + settings that differ from the default. + """ + data, found = _check(text, index, name) + block = found[index] + lines = _lines(text) + own = range(block.header + 1, block.own_end) + at = [n for n in own if _ENABLED.match(lines[n]) and not _in_string(text, n)] + if enabled: + new = [line for n, line in enumerate(lines) if n not in at] + else: + new = list(lines) + if at: + new[at[0]] = "enabled = false\n" + else: + after = _setting_line(lines, own, "type") or _setting_line(lines, own, "name") + new.insert((after or block.header) + 1, "enabled = false\n") + expected = plugin_blocks(data) + changed = {k: v for k, v in expected[index].items() if k != "enabled"} + expected[index] = changed if enabled else changed | {"enabled": False} + return _checked("".join(new), data | {"plugin": expected}) + + +def add(text: str, block_text: str) -> str: + """``text`` with ``block_text`` (one ``[[plugin]]`` block, as written by + :func:`paperpi.example.plugin_block`) added after the last plugin block.""" + data = load(text) + found = blocks(text) + if len(found) != len(plugin_blocks(data)): + raise EditError(_UNKNOWN_FORM) + new_block = plugin_blocks(load(block_text)) + lines = _lines(text) + at = found[-1].end if found else len(lines) + before = "".join(lines[:at]) + after = "".join(lines[at:]) + gap = "" if not before or before.endswith("\n\n") else "\n" + new = before + gap + block_text + ("\n" + after if after else "") + return _checked(new, data | {"plugin": [*plugin_blocks(data), *new_block]}) + + +_UNKNOWN_FORM = ( + "the plugin blocks in the config file are written in a way the web interface can't " + "change safely; change it by hand" +) + + +def _check(text: str, index: int, name: str) -> tuple[dict[str, Any], list[Block]]: + data = load(text) + found = blocks(text) + raw = plugin_blocks(data) + if len(found) != len(raw): + raise EditError(_UNKNOWN_FORM) + if not 0 <= index < len(raw): + raise ChangedMeanwhile(_MEANWHILE) + if name_of(raw[index]) != name: + raise ChangedMeanwhile(_MEANWHILE) + return data, found + + +_MEANWHILE = "the config file was changed meanwhile; look at the list again and retry" + + +def _checked(new: str, expected: dict[str, Any]) -> str: + """``new``, after checking that PaperPi reads it back as ``expected``.""" + try: + read_back = tomllib.loads(new) + except tomllib.TOMLDecodeError: + read_back = None + if read_back != expected: + raise EditError(_UNKNOWN_FORM) + return new + + +def _lines(text: str) -> list[str]: + """The lines of ``text``, each ending with a line break.""" + return [line if line.endswith("\n") else line + "\n" for line in text.splitlines(True)] + + +def _spaced(chunk: list[str]) -> list[str]: + """``chunk`` ending with a blank line, so it stays apart from the block after it.""" + return chunk if chunk and not chunk[-1].strip() else [*chunk, "\n"] + + +def _setting_line(lines: list[str], within: range, key: str) -> int | None: + pattern = re.compile(rf"\s*{key}\s*=") + return next((n for n in within if pattern.match(lines[n])), None) + + +def _in_string(text: str, line: int) -> bool: + """True when line ``line`` starts inside a multi-line string.""" + before = "\n".join(text.splitlines()[:line]) + quote = None + for found in re.finditer(r'"""|\'\'\'', before): + if quote is None: + quote = found.group() + elif found.group() == quote: + quote = None + return quote is not None diff --git a/src/paperpi/web/password.py b/src/paperpi/web/password.py index 972f0fa..5f26208 100644 --- a/src/paperpi/web/password.py +++ b/src/paperpi/web/password.py @@ -13,17 +13,14 @@ import base64 import hashlib import hmac -import os import secrets -import stat import tomllib from pathlib import Path import tomlkit from tomlkit.exceptions import TOMLKitError -from .. import config -from ..files import write_atomic +from . import config_file #: Shortest and longest password the web interface accepts. PASSWORD_LENGTH = (8, 1000) @@ -99,11 +96,9 @@ def save_password_hash(path: Path, stored: str | None) -> bool: permissions. Returns False when there was nothing to change. Raises :class:`PasswordError` when the file can't be read, changed or written. """ - # A config file that is a link to another file: change the file it points to. - path = Path(os.path.realpath(path)) try: - text = config.read_text(path) - except config.ConfigError as error: + path, text = config_file.read(path) + except config_file.EditError as error: raise PasswordError(str(error)) from None try: document = tomlkit.parse(text) @@ -131,13 +126,9 @@ def save_password_hash(path: Path, stored: str | None) -> bool: ): raise PasswordError(f"{path} can't be written back with the new password") try: - # The new file keeps the old one's permissions and, when run with sudo, its owner, - # so the PaperPi service can still read it. - info = os.stat(path) - owner = (info.st_uid, info.st_gid) if os.geteuid() == 0 else None - write_atomic(path, new_text.encode("utf-8"), mode=stat.S_IMODE(info.st_mode), owner=owner) - except OSError as error: - raise PasswordError(f"can't save {path}: {error}") from None + config_file.write(path, new_text) + except config_file.EditError as error: + raise PasswordError(str(error)) from None return True diff --git a/tests/test_web_config_file.py b/tests/test_web_config_file.py new file mode 100644 index 0000000..e8eb2d5 --- /dev/null +++ b/tests/test_web_config_file.py @@ -0,0 +1,129 @@ +import tomllib + +import pytest + +from paperpi import example, plugins +from paperpi.web import config_file +from paperpi.web.config_file import ChangedMeanwhile, EditError + +CONFIG = """\ +config_version = 1 +[display] +type = "virtual" + +# the clock in the kitchen +[[plugin]] +name = "Clock" +type = "basic_clock" +# refresh = 60 + +# words +[[plugin]] +name = "Words" +type = "word_clock" +enabled = true # for now +[plugin.extra] +a = 1 + +[web] +port = 8081 +""" + + +def names(text): + return [b["name"] for b in tomllib.loads(text).get("plugin", [])] + + +def test_blocks_start_at_their_comments(): + found = config_file.blocks(CONFIG) + lines = CONFIG.splitlines() + assert [lines[b.start] for b in found] == ["# the clock in the kitchen", "# words"] + assert [lines[b.header] for b in found] == ["[[plugin]]", "[[plugin]]"] + # The second block holds its [plugin.extra] part, not [web]. + assert lines[found[1].own_end] == "[plugin.extra]" + assert lines[found[1].end] == "[web]" + + +def test_moving_a_block_takes_its_comments_and_parts_along(): + moved = config_file.move(CONFIG, 1, "Words", -1) + assert names(moved) == ["Words", "Clock"] + assert moved.index("# words") < moved.index('name = "Words"') < moved.index("[plugin.extra]") + assert moved.index("[plugin.extra]") < moved.index("# the clock in the kitchen") + assert tomllib.loads(moved)["plugin"][0]["extra"] == {"a": 1} + assert tomllib.loads(moved)["web"] == {"port": 8081} + # And back again: the same settings, nothing lost. + back = config_file.move(moved, 0, "Words", 1) + assert tomllib.loads(back) == tomllib.loads(CONFIG) + for comment in ["# the clock in the kitchen", "# refresh = 60", "# words", "# for now"]: + assert back.count(comment) == 1 + + +def test_a_block_cant_move_past_the_ends(): + with pytest.raises(EditError, match="can't move further"): + config_file.move(CONFIG, 0, "Clock", -1) + with pytest.raises(EditError, match="can't move further"): + config_file.move(CONFIG, 1, "Words", 1) + + +def test_removing_a_block_removes_its_comments(): + removed = config_file.remove(CONFIG, 0, "Clock") + assert names(removed) == ["Words"] + assert "kitchen" not in removed and "# refresh" not in removed + gone = config_file.remove(removed, 0, "Words") + assert "plugin" not in tomllib.loads(gone) + assert tomllib.loads(gone)["web"] == {"port": 8081} + + +def test_switching_off_and_on(): + off = config_file.set_enabled(CONFIG, 0, "Clock", False) + assert 'type = "basic_clock"\nenabled = false\n' in off + assert config_file.set_enabled(off, 0, "Clock", True) == CONFIG + # An "enabled" line written by hand is changed; switching on removes it (the default). + words_off = config_file.set_enabled(CONFIG, 1, "Words", False) + assert tomllib.loads(words_off)["plugin"][1]["enabled"] is False + assert ( + "enabled" + not in tomllib.loads(config_file.set_enabled(words_off, 1, "Words", True))["plugin"][1] + ) + + +def test_adding_a_block_after_the_last_plugin(): + block = example.plugin_block(plugins.load("dec_binary_clock"), "Dots", {}) + added = config_file.add(CONFIG, block) + assert names(added) == ["Clock", "Words", "Dots"] + assert added.index('name = "Dots"') < added.index("[web]") + assert tomllib.loads(added)["web"] == {"port": 8081} + # A file without plugins: at the end. + empty = 'config_version = 1\n[display]\ntype = "virtual"' + assert names(config_file.add(empty, block)) == ["Dots"] + + +def test_a_block_changed_meanwhile_is_not_touched(): + with pytest.raises(ChangedMeanwhile): + config_file.remove(CONFIG, 0, "Words") + with pytest.raises(ChangedMeanwhile): + config_file.move(CONFIG, 5, "Clock", 1) + + +def test_files_it_doesnt_understand_are_not_changed(): + # A [[plugin]] line inside a multi-line string is not a block... + text = CONFIG.replace('type = "virtual"', 'type = "virtual"\nnote = """\n[[plugin]]\n"""') + assert names(config_file.remove(text, 0, "Clock")) == ["Words"] + # ... and a header written in another way is not found, so nothing is changed. + quoted = CONFIG.replace('[[plugin]]\nname = "Words"', '[["plugin"]]\nname = "Words"') + with pytest.raises(EditError, match="change it by hand"): + config_file.move(quoted, 0, "Clock", 1) + with pytest.raises(EditError, match="not valid TOML"): + config_file.move("[[plugin]\n", 0, "Clock", 1) + + +def test_saving_keeps_the_permissions(tmp_path): + path = tmp_path / "paperpi.toml" + path.write_text(CONFIG) + path.chmod(0o640) + real, text = config_file.read(path) + config_file.write(real, config_file.remove(text, 0, "Clock")) + assert names(path.read_text()) == ["Words"] + assert path.stat().st_mode & 0o777 == 0o640 + with pytest.raises(EditError, match="file not found"): + config_file.read(tmp_path / "nothing.toml") From 0f5eae66c198ad9b7a154659363dca5b6c9ec8d2 Mon Sep 17 00:00:00 2001 From: txoof-bot <337660340+txoof-bot@users.noreply.github.com> Date: Wed, 7 Oct 2026 18:22:14 +0200 Subject: [PATCH 4/4] Review fixes: line breaks, Windows line endings, size limit, shared lock Co-Authored-By: Claude Opus 5.5 --- docs/decisions/config-format.md | 2 +- src/paperpi/config.py | 4 +- src/paperpi/plugin.py | 3 +- src/paperpi/web/config_file.py | 207 +++++++++++++++++++------------- src/paperpi/web/password.py | 5 + tests/test_example.py | 4 + tests/test_web_config_file.py | 83 ++++++++++++- 7 files changed, 223 insertions(+), 85 deletions(-) diff --git a/docs/decisions/config-format.md b/docs/decisions/config-format.md index f6bcc6a..535b500 100644 --- a/docs/decisions/config-format.md +++ b/docs/decisions/config-format.md @@ -92,7 +92,7 @@ The web interface only accepts valid values, so most of these mistakes can only ### Writing the file - The web interface keeps comments that were written by hand. - *Update (M5 part 2b, issue #238):* `[[plugin]]` blocks are moved, removed, switched on or off and added by changing the file's text one block at a time (`paperpi.web.config_file`), not with tomlkit: tomlkit counts the comment lines just above a `[[plugin]]` line as part of the block before it, so they would move with the wrong plugin. Here they belong to the block below them. Every change is read back and compared before it is saved; a file written in a way this code doesn't understand is not changed, and the web interface says to change it by hand. Single settings (such as the password) are still written with tomlkit. + *Update (M5 part 2b, issue #238):* `[[plugin]]` blocks are moved, removed, switched on or off and added by changing the file's text one block at a time (`paperpi.web.config_file`), not with tomlkit: tomlkit counts the comment lines just above a `[[plugin]]` line as part of the block before it, so they would move with the wrong plugin. Here they belong to the block below them (when a blank line is above them), and they are removed with it; a comment on the same line as `enabled =` is lost when the plugin is switched on or off. Every change is read back and compared before it is saved; a file written in a way this code doesn't understand is not changed, and the web interface says to change it by hand. A change that would make the file larger than PaperPi reads is refused. Single settings (such as the password) are still written with tomlkit. - The file is written safely: first to a temporary file, then that file replaces the old one in one step. A power cut during a save can't leave a half-written config. ### Secrets diff --git a/src/paperpi/config.py b/src/paperpi/config.py index 35af6fb..9ee7674 100644 --- a/src/paperpi/config.py +++ b/src/paperpi/config.py @@ -800,7 +800,9 @@ def _key_lines(text: str) -> dict[tuple, int]: counts: dict[str, int] = {} section: tuple = () quote = None # inside a multi-line string: the quotes it started with (""" or ''') - for number, line in enumerate(text.splitlines(), start=1): + # Split at \n only, like TOML: splitlines() also splits at U+2028 and others, which TOML + # allows inside strings and comments. + for number, line in enumerate(text.split("\n"), start=1): starts_inside = quote is not None for found in re.finditer(r'"""|\'\'\'', line): if quote is None: diff --git a/src/paperpi/plugin.py b/src/paperpi/plugin.py index d17589f..8d68b9c 100644 --- a/src/paperpi/plugin.py +++ b/src/paperpi/plugin.py @@ -212,7 +212,8 @@ class PluginEntry(BaseModel): def _no_control_characters(cls, name: str) -> str: # They can't be written back to the file reliably, and would change the terminal's # output in ``paperpi list``. - if re.search(r"[\x00-\x1f\x7f]", name): + # Also the characters Python counts as line breaks (U+0085, U+2028, U+2029). + if re.search(r"[\x00-\x1f\x7f-\x9f\u2028\u2029]", name): raise ValueError("must not hold control characters (such as tab or new line)") return name diff --git a/src/paperpi/web/config_file.py b/src/paperpi/web/config_file.py index 7f6c287..0223cec 100644 --- a/src/paperpi/web/config_file.py +++ b/src/paperpi/web/config_file.py @@ -1,13 +1,19 @@ """Changing the config file from the web interface, keeping everything else in it as it is. - :func:`read` and :func:`write` read the file and save a changed copy that keeps the old - file's permissions (and owner, when run as root). + file's permissions (and owner, when run as root). Hold :data:`LOCK` from reading to + saving, so two changes at the same moment (a plugin and the password) can't undo each + other. :func:`saved_here` tells whether a text is the one the web interface saved last. - The plugin changes (:func:`move`, :func:`remove`, :func:`set_enabled`, :func:`add`) work on the file's text, one ``[[plugin]]`` block at a time. The comment lines just above a - ``[[plugin]]`` line belong to that block and move with it. + ``[[plugin]]`` line (with a blank line above them) belong to that block: they move with it + and are removed with it. A comment on the same line as ``enabled =`` is lost when the + plugin is switched on or off. - Every change is checked before it is saved: PaperPi must read the new text back as exactly the old settings plus the change. A file written in a way these functions don't - understand is never saved wrongly; :class:`EditError` says so instead. + understand (for example a ``[[plugin]]`` line written differently, or a value line + that looks like a ``[part]`` line) is never saved wrongly: :class:`UnknownForm` says to + change it by hand instead. - Each change names its block by place and name, so a block that was moved or removed by hand a moment ago is not changed by mistake (:class:`ChangedMeanwhile`). """ @@ -17,14 +23,19 @@ import os import re import stat +import threading import tomllib from dataclasses import dataclass from pathlib import Path from typing import Any -from .. import config +from .. import config, limits from ..files import write_atomic +#: Held from reading the config file to saving the changed copy. +LOCK = threading.Lock() +_saved: dict[Path, str] = {} + class EditError(ValueError): """The config file can't be read, changed or saved; the message says why.""" @@ -33,6 +44,21 @@ class EditError(ValueError): class ChangedMeanwhile(EditError): """The block to change is not where the page showed it: the file changed meanwhile.""" + def __init__(self) -> None: + super().__init__( + "The config file changed after this page was opened. Check the list and try again." + ) + + +class UnknownForm(EditError): + """The file is written in a way these functions can't change safely.""" + + def __init__(self) -> None: + super().__init__( + "The plugin list in the config file is written in a way the web interface can't " + "change safely. Make this change in the config file itself." + ) + def read(path: Path) -> tuple[Path, str]: """The real path of the config file (links followed) and its text.""" @@ -46,14 +72,26 @@ def read(path: Path) -> tuple[Path, str]: def write(path: Path, text: str) -> None: """Save ``text`` as the config file at ``path`` (a real path, from :func:`read`).""" + data = text.encode("utf-8") + if len(data) > limits.CONFIG_FILE_BYTES: + raise EditError( + f"The config file would be larger than {limits.CONFIG_FILE_BYTES} bytes, which " + "PaperPi can't read. Remove a plugin first." + ) try: # The new file keeps the old one's permissions and, when run with sudo, its owner, # so the PaperPi service can still read it. info = os.stat(path) owner = (info.st_uid, info.st_gid) if os.geteuid() == 0 else None - write_atomic(path, text.encode("utf-8"), mode=stat.S_IMODE(info.st_mode), owner=owner) + write_atomic(path, data, mode=stat.S_IMODE(info.st_mode), owner=owner) except OSError as error: raise EditError(f"can't save {path}: {error}") from None + _saved[path] = text + + +def saved_here(path: Path, text: str) -> bool: + """True when ``text`` is what the web interface saved last as the config file ``path``.""" + return _saved.get(Path(os.path.realpath(path))) == text def load(text: str) -> dict[str, Any]: @@ -79,15 +117,28 @@ class Block: _HEADER = re.compile(r"\s*(\[\[?)\s*([A-Za-z0-9_.-]+)\s*\]\]?\s*(#.*)?") -_ENABLED = re.compile(r"\s*enabled\s*=") -def blocks(text: str) -> list[Block]: - """The ``[[plugin]]`` blocks in ``text``, in file order.""" - lines = text.splitlines() +@dataclass(frozen=True) +class _Scan: + lines: list[str] + """The lines, each with its line break (TOML only breaks lines at ``\\n``).""" + in_string: frozenset[int] + """Lines that start inside a multi-line string.""" + blocks: list[Block] + + +def _scan(text: str) -> _Scan: + lines = re.split(r"(?<=\n)", text) + if lines and lines[-1] == "": + lines.pop() + plain = [line.rstrip("\r\n") for line in lines] starts: list[tuple[int, str]] = [] # (line, "plugin" / "part of plugin" / "other") + in_string = set() quote = None # inside a multi-line string: the quotes it started with - for number, line in enumerate(lines): + for number, line in enumerate(plain): + if quote is not None: + in_string.add(number) inside = quote is not None for found in re.finditer(r'"""|\'\'\'', line): if quote is None: @@ -110,16 +161,25 @@ def blocks(text: str) -> list[Block]: later = starts[index + 1 :] own_end = later[0][0] if later else len(lines) after = next((n for n, k in later if k != "part of plugin"), len(lines)) - end = _comments_above(lines, after, header + 1) if after < len(lines) else after + end = _comments_above(plain, after, header + 1) if after < len(lines) else after lowest = max((n + 1 for n, _ in starts[:index]), default=0) - found.append(Block(_comments_above(lines, header, lowest), header, own_end, end)) - return found + found.append(Block(_comments_above(plain, header, lowest), header, own_end, end)) + return _Scan(lines, frozenset(in_string), found) + + +def blocks(text: str) -> list[Block]: + """The ``[[plugin]]`` blocks in ``text``, in file order.""" + return _scan(text).blocks def _comments_above(lines: list[str], line: int, lowest: int) -> int: - """The first of the comment lines just above ``line`` (or ``line`` itself).""" - while line > lowest and lines[line - 1].lstrip().startswith("#"): - line -= 1 + """The first of the comment lines just above ``line``, when a blank line (or the start + of the file) is above them; else ``line`` itself: then they belong to what is above.""" + start = line + while start > lowest and lines[start - 1].lstrip().startswith("#"): + start -= 1 + if start == line or start == 0 or not lines[start - 1].strip(): + return start return line @@ -137,16 +197,16 @@ def name_of(block: Any) -> str: def move(text: str, index: int, name: str, step: int) -> str: """``text`` with plugin block ``index`` moved one place up (``step=-1``) or down (``1``).""" - data, found = _check(text, index, name) + data, scan = _check(text, index, name) other = index + step - if step not in (-1, 1) or not 0 <= other < len(found): - raise EditError("this plugin can't move further") + if step not in (-1, 1) or not 0 <= other < len(scan.blocks): + raise EditError("This plugin can't move further.") first, second = sorted((index, other)) - a, b = found[first], found[second] - lines = _lines(text) + a, b = scan.blocks[first], scan.blocks[second] + lines = _ended(scan.lines) new = [ *lines[: a.start], - *_spaced(lines[b.start : b.end]), + *_spaced(lines[b.start : b.end], _newline(lines)), *lines[a.end : b.start], *lines[a.start : a.end], *lines[b.end :], @@ -157,11 +217,10 @@ def move(text: str, index: int, name: str, step: int) -> str: def remove(text: str, index: int, name: str) -> str: - """``text`` without plugin block ``index``.""" - data, found = _check(text, index, name) - block = found[index] - lines = _lines(text) - new = "".join([*lines[: block.start], *lines[block.end :]]) + """``text`` without plugin block ``index`` (and the comments just above it).""" + data, scan = _check(text, index, name) + block = scan.blocks[index] + new = "".join([*scan.lines[: block.start], *scan.lines[block.end :]]) expected = plugin_blocks(data) del expected[index] data = {k: v for k, v in data.items() if k != "plugin"} @@ -174,20 +233,21 @@ def set_enabled(text: str, index: int, name: str, enabled: bool) -> str: Off writes ``enabled = false``; on removes that line, because the file only holds settings that differ from the default. """ - data, found = _check(text, index, name) - block = found[index] - lines = _lines(text) - own = range(block.header + 1, block.own_end) - at = [n for n in own if _ENABLED.match(lines[n]) and not _in_string(text, n)] + data, scan = _check(text, index, name) + block = scan.blocks[index] + lines = _ended(scan.lines) + own = [n for n in range(block.header + 1, block.own_end) if n not in scan.in_string] + at = [n for n in own if re.match(r"\s*enabled\s*=", lines[n])] + line = "enabled = false" + _newline(lines) if enabled: - new = [line for n, line in enumerate(lines) if n not in at] + new = [kept for n, kept in enumerate(lines) if n not in at] else: new = list(lines) if at: - new[at[0]] = "enabled = false\n" + new[at[0]] = line else: after = _setting_line(lines, own, "type") or _setting_line(lines, own, "name") - new.insert((after or block.header) + 1, "enabled = false\n") + new.insert((after or block.header) + 1, line) expected = plugin_blocks(data) changed = {k: v for k, v in expected[index].items() if k != "enabled"} expected[index] = changed if enabled else changed | {"enabled": False} @@ -198,39 +258,29 @@ def add(text: str, block_text: str) -> str: """``text`` with ``block_text`` (one ``[[plugin]]`` block, as written by :func:`paperpi.example.plugin_block`) added after the last plugin block.""" data = load(text) - found = blocks(text) - if len(found) != len(plugin_blocks(data)): - raise EditError(_UNKNOWN_FORM) + scan = _scan(text) + if len(scan.blocks) != len(plugin_blocks(data)): + raise UnknownForm() new_block = plugin_blocks(load(block_text)) - lines = _lines(text) - at = found[-1].end if found else len(lines) - before = "".join(lines[:at]) - after = "".join(lines[at:]) - gap = "" if not before or before.endswith("\n\n") else "\n" - new = before + gap + block_text + ("\n" + after if after else "") + lines = _ended(scan.lines) + newline = _newline(lines) + at = scan.blocks[-1].end if scan.blocks else len(lines) + before, after = "".join(lines[:at]), "".join(lines[at:]) + gap = "" if not before or before.endswith(newline * 2) else newline + block_text = block_text.replace("\n", newline) + new = before + gap + block_text + (newline + after if after else "") return _checked(new, data | {"plugin": [*plugin_blocks(data), *new_block]}) -_UNKNOWN_FORM = ( - "the plugin blocks in the config file are written in a way the web interface can't " - "change safely; change it by hand" -) - - -def _check(text: str, index: int, name: str) -> tuple[dict[str, Any], list[Block]]: +def _check(text: str, index: int, name: str) -> tuple[dict[str, Any], _Scan]: data = load(text) - found = blocks(text) + scan = _scan(text) raw = plugin_blocks(data) - if len(found) != len(raw): - raise EditError(_UNKNOWN_FORM) - if not 0 <= index < len(raw): - raise ChangedMeanwhile(_MEANWHILE) - if name_of(raw[index]) != name: - raise ChangedMeanwhile(_MEANWHILE) - return data, found - - -_MEANWHILE = "the config file was changed meanwhile; look at the list again and retry" + if len(scan.blocks) != len(raw): + raise UnknownForm() + if not 0 <= index < len(raw) or name_of(raw[index]) != name: + raise ChangedMeanwhile() + return data, scan def _checked(new: str, expected: dict[str, Any]) -> str: @@ -240,32 +290,27 @@ def _checked(new: str, expected: dict[str, Any]) -> str: except tomllib.TOMLDecodeError: read_back = None if read_back != expected: - raise EditError(_UNKNOWN_FORM) + raise UnknownForm() return new -def _lines(text: str) -> list[str]: - """The lines of ``text``, each ending with a line break.""" - return [line if line.endswith("\n") else line + "\n" for line in text.splitlines(True)] +def _ended(lines: list[str]) -> list[str]: + """``lines``, the last one with a line break too.""" + if lines and not lines[-1].endswith("\n"): + return [*lines[:-1], lines[-1] + _newline(lines)] + return lines + + +def _newline(lines: list[str]) -> str: + """The file's line break: ``\\r\\n`` (Windows) or ``\\n``.""" + return "\r\n" if lines and lines[0].endswith("\r\n") else "\n" -def _spaced(chunk: list[str]) -> list[str]: +def _spaced(chunk: list[str], newline: str) -> list[str]: """``chunk`` ending with a blank line, so it stays apart from the block after it.""" - return chunk if chunk and not chunk[-1].strip() else [*chunk, "\n"] + return chunk if chunk and not chunk[-1].strip() else [*chunk, newline] -def _setting_line(lines: list[str], within: range, key: str) -> int | None: +def _setting_line(lines: list[str], within: list[int], key: str) -> int | None: pattern = re.compile(rf"\s*{key}\s*=") return next((n for n in within if pattern.match(lines[n])), None) - - -def _in_string(text: str, line: int) -> bool: - """True when line ``line`` starts inside a multi-line string.""" - before = "\n".join(text.splitlines()[:line]) - quote = None - for found in re.finditer(r'"""|\'\'\'', before): - if quote is None: - quote = found.group() - elif found.group() == quote: - quote = None - return quote is not None diff --git a/src/paperpi/web/password.py b/src/paperpi/web/password.py index 5f26208..f50fa6d 100644 --- a/src/paperpi/web/password.py +++ b/src/paperpi/web/password.py @@ -96,6 +96,11 @@ def save_password_hash(path: Path, stored: str | None) -> bool: permissions. Returns False when there was nothing to change. Raises :class:`PasswordError` when the file can't be read, changed or written. """ + with config_file.LOCK: + return _save_password_hash(path, stored) + + +def _save_password_hash(path: Path, stored: str | None) -> bool: try: path, text = config_file.read(path) except config_file.EditError as error: diff --git a/tests/test_example.py b/tests/test_example.py index b2c2284..916d13b 100644 --- a/tests/test_example.py +++ b/tests/test_example.py @@ -117,6 +117,10 @@ def test_block_refuses_what_paperpi_could_not_read_back(): # tomlkit writes the control character ESC in a form Python's TOML reader doesn't know. with pytest.raises(ValueError, match="control characters"): plugin_block(plugins.load("basic_clock"), "B\x1bad") + # Characters Python counts as line breaks, which the web interface's line counting skips. + for name in ["a\u2028b", "a\u2029b", "a\x85b"]: + with pytest.raises(ValueError, match="control characters"): + plugin_block(plugins.load("basic_clock"), name) with pytest.raises(ValueError, match="reads it back"): plugin_block(plugins.load("met_no"), "Weather", {"place": "B\x1bad"}) diff --git a/tests/test_web_config_file.py b/tests/test_web_config_file.py index e8eb2d5..d5f6baa 100644 --- a/tests/test_web_config_file.py +++ b/tests/test_web_config_file.py @@ -111,7 +111,7 @@ def test_files_it_doesnt_understand_are_not_changed(): assert names(config_file.remove(text, 0, "Clock")) == ["Words"] # ... and a header written in another way is not found, so nothing is changed. quoted = CONFIG.replace('[[plugin]]\nname = "Words"', '[["plugin"]]\nname = "Words"') - with pytest.raises(EditError, match="change it by hand"): + with pytest.raises(EditError, match="config file itself"): config_file.move(quoted, 0, "Clock", 1) with pytest.raises(EditError, match="not valid TOML"): config_file.move("[[plugin]\n", 0, "Clock", 1) @@ -127,3 +127,84 @@ def test_saving_keeps_the_permissions(tmp_path): assert path.stat().st_mode & 0o777 == 0o640 with pytest.raises(EditError, match="file not found"): config_file.read(tmp_path / "nothing.toml") + + +def test_comments_right_after_a_setting_stay_with_the_block_above(): + text = ( + '[[plugin]]\nname = "A"\ntype = "x"\n# refresh = 60\n[[plugin]]\nname = "B"\ntype = "y"\n' + ) + assert ( + config_file.remove(text, 1, "B") == '[[plugin]]\nname = "A"\ntype = "x"\n# refresh = 60\n' + ) + + +def test_windows_line_endings_are_kept(): + text = CONFIG.replace("\n", "\r\n") + block = example.plugin_block(plugins.load("dec_binary_clock"), "Dots", {}) + for changed in [ + config_file.move(text, 1, "Words", -1), + config_file.set_enabled(text, 0, "Clock", False), + config_file.set_enabled(text, 1, "Words", True), + config_file.remove(text, 0, "Clock"), + config_file.add(text, block), + ]: + assert "\n" not in changed.replace("\r\n", "") + + +def test_a_file_without_a_last_line_break(): + text = CONFIG.removesuffix("\n") + assert tomllib.loads(config_file.move(text, 1, "Words", -1))["web"] == {"port": 8081} + last = '[[plugin]]\nname = "A"\ntype = "x"' + off = config_file.set_enabled(last, 0, "A", False) + assert tomllib.loads(off)["plugin"][0]["enabled"] is False + + +def test_other_parts_between_blocks_stay_where_they_are(): + text = CONFIG.replace("[web]\nport = 8081\n", "").replace( + "# words\n", "[web]\nport = 8081\n\n# words\n" + ) + moved = config_file.move(text, 1, "Words", -1) + assert names(moved) == ["Words", "Clock"] + # The blocks swap places; [web] stays between them. + assert moved.index('name = "Words"') < moved.index("[web]") < moved.index('name = "Clock"') + assert tomllib.loads(moved)["web"] == {"port": 8081} + + +def test_enabled_in_a_part_or_a_string_is_not_the_switch(): + text = ( + '[[plugin]]\nname = "A"\nnote = """\nenabled = true\n"""\ntype = "x"\n' + "[plugin.extra]\nenabled = true\n" + ) + off = config_file.set_enabled(text, 0, "A", False) + data = tomllib.loads(off)["plugin"][0] + assert data["enabled"] is False and data["extra"] == {"enabled": True} + assert data["note"] == "enabled = true\n" + assert config_file.set_enabled(off, 0, "A", True) == text + # Only a "type" inside a string: the new line goes right below [[plugin]]. + hidden = '[[plugin]]\nnote = """\ntype = "y"\n"""\nname = "B"\n' + assert tomllib.loads(config_file.set_enabled(hidden, 0, "B", False))["plugin"][0] == { + "note": 'type = "y"\n', + "name": "B", + "enabled": False, + } + + +def test_unicode_line_breaks_inside_a_value_are_not_line_breaks(): + text = CONFIG.replace('name = "Words"', 'name = "Words"\nplace = "a
b\x85c"') + assert names(config_file.remove(text, 0, "Clock")) == ["Words"] + off = config_file.set_enabled(text, 1, "Words", False) + assert tomllib.loads(off)["plugin"][1]["place"] == "a
b\x85c" + + +def test_a_file_too_large_for_paperpi_is_not_saved(tmp_path, monkeypatch): + from paperpi import limits + + path = tmp_path / "paperpi.toml" + path.write_text(CONFIG) + monkeypatch.setattr(limits, "CONFIG_FILE_BYTES", len(CONFIG)) + with pytest.raises(EditError, match="larger than"): + config_file.write(path, CONFIG + "#\n") + assert path.read_text() == CONFIG + config_file.write(path, CONFIG) + assert config_file.saved_here(path, CONFIG) + assert not config_file.saved_here(path, CONFIG + "#\n")