feat(templates): default templates + TemplateLoader (Phase 4)#56
Conversation
schema_version defaults to 1 and is the hook for migrating older YAML payloads when the layout schema grows in future iterations. Refs #22
Templates may reference any registered plugin name (validated at load time by TemplateLoader against IntegrationRegistry). app=None marks a generic template (QR-only, no integration-specific fields). New integrations no longer require a schema change. Refs #22
pyyaml>=6.0 added to runtime dependencies (was installed but undeclared). types-PyYAML added to dev dependencies so mypy --strict passes on the import in template_loader.py. Refs #22
_load_single parses one YAML file, validates against TemplateSchema, and checks the app reference against IntegrationRegistry. app=null is allowed for generic templates and skips the registry check. Refs #22
app: null marks these as integration-agnostic — they render any LabelData, useful for inventory scanning without app-specific fields. Refs #22
13 tests (12 parametrized + 1 sanity check on the set of shipped ids). Fails the build if any template YAML breaks any contract: schema, registry, geometry, or renderer. Refs #22
main.py imports app.integrations (triggers plugin discovery) and calls TemplateLoader.load_dir on the seed directory. After this, TemplateLoader.all() returns the 12 shipped templates. Refs #22
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request completes Phase 4 of the label rendering project by implementing a robust template loading mechanism and shipping a set of default templates. The changes decouple template definitions from hardcoded schema constraints, allowing for easier extension of integrations and future-proofing via versioned schemas. The system now validates templates at startup, ensuring configuration errors are caught early. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a template loading system that uses YAML files to define label layouts. It includes a new TemplateLoader service, updates to the TemplateSchema to support versioning and generic templates, and a set of seed templates for various integrations (Snipe-IT, Spoolman, Grocy). Feedback focuses on improving the robustness and atomicity of the TemplateLoader, specifically regarding error handling during file reads, ensuring atomic updates to the template cache to prevent inconsistent states on failure, and adding checks for duplicate template IDs.
| try: | ||
| raw = yaml.safe_load(path.read_text()) | ||
| except yaml.YAMLError as e: | ||
| raise TemplateValidationError(f"{path.name}: YAML parse error: {e}") from e |
There was a problem hiding this comment.
The current error handling only catches yaml.YAMLError. However, path.read_text() can raise an OSError (e.g., if the file is unreadable due to permissions), which would bypass the TemplateValidationError wrapper and crash the loader. Additionally, it is safer to explicitly specify encoding="utf-8" to ensure consistent behavior across different platforms.
| try: | |
| raw = yaml.safe_load(path.read_text()) | |
| except yaml.YAMLError as e: | |
| raise TemplateValidationError(f"{path.name}: YAML parse error: {e}") from e | |
| try: | |
| raw = yaml.safe_load(path.read_text(encoding="utf-8")) | |
| except (OSError, yaml.YAMLError) as e: | |
| raise TemplateValidationError(f"{path.name}: load error: {e}") from e |
| for path in sorted(directory.glob("*.yaml")): | ||
| template = cls._load_single(path) | ||
| cls._cache[template.id] = template |
There was a problem hiding this comment.
The implementation of load_dir is not atomic, which contradicts the docstring's claim (lines 60-61) that the cache is left in its previous state on failure. Currently, if a failure occurs on the Nth file, the previous N-1 files remain in the cache. Furthermore, the loader should check for duplicate template IDs within the same directory to prevent silent overwrites.
| for path in sorted(directory.glob("*.yaml")): | |
| template = cls._load_single(path) | |
| cls._cache[template.id] = template | |
| loaded: dict[str, TemplateSchema] = {} | |
| for path in sorted(directory.glob("*.yaml")): | |
| template = cls._load_single(path) | |
| if template.id in loaded: | |
| raise TemplateValidationError(f"{path.name}: duplicate template id {template.id!r}") | |
| loaded[template.id] = template | |
| cls._cache.update(loaded) |
| cls._cache.clear() | ||
| cls.load_dir(directory) |
There was a problem hiding this comment.
Calling cls._cache.clear() before load_dir makes reload non-atomic. If load_dir fails (e.g., due to a syntax error in a new YAML file), the cache will be left empty rather than being restored to its previous valid state. To ensure atomicity, you should load all templates into a temporary dictionary first and then replace the cache only if all files are valid.
…cate ids Addresses Gemini review findings on PR #56: - _load_single: wrap path.read_text in OSError handler so permission errors / unreadable files surface as TemplateValidationError; specify encoding=utf-8 explicitly for cross-platform consistency - load_dir: parse into a staging dict and replace the cache only after the full directory parses cleanly. A failure on file N no longer leaves files 1..N-1 in a partial cache. - load_dir: reject duplicate ids across files instead of silently overwriting (a real authoring bug should fail loudly). - reload: delegate to the now-atomic load_dir, so a broken mid-edit YAML (Phase-7 editor scenario) preserves the previous valid cache instead of wiping it. Refs #22
There was a problem hiding this comment.
Pull request overview
Adds a YAML-based default template system to the backend: introduces a TemplateLoader that loads/validates templates against the integration plugin registry, ships 12 seed templates (Snipe-IT/Spoolman/Grocy + generic QR-only across 12/18/24mm), and adds unit tests to enforce schema/loader/seed rendering contracts.
Changes:
- Introduces
TemplateLoader(YAML parsing, schema validation, integration-registry validation, cache helpers). - Updates
TemplateSchemato supportapp: str | Noneand addsschema_versionfor future migrations. - Adds 12 seed template YAMLs + tests to ensure shipped templates load and render.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/app/main.py | Triggers integration discovery import and loads seed templates at startup. |
| backend/app/schemas/template.py | Relaxes app typing to str | None and adds schema_version to the template schema. |
| backend/app/seed/init.py | Marks seed data as a package. |
| backend/app/seed/templates/grocy-12mm.yaml | Adds Grocy 12mm default template YAML. |
| backend/app/seed/templates/grocy-18mm.yaml | Adds Grocy 18mm default template YAML. |
| backend/app/seed/templates/grocy-24mm.yaml | Adds Grocy 24mm default template YAML. |
| backend/app/seed/templates/qr-only-12mm.yaml | Adds generic QR-only 12mm template YAML. |
| backend/app/seed/templates/qr-only-18mm.yaml | Adds generic QR-only 18mm template YAML. |
| backend/app/seed/templates/qr-only-24mm.yaml | Adds generic QR-only 24mm template YAML. |
| backend/app/seed/templates/snipeit-12mm.yaml | Adds Snipe-IT 12mm default template YAML. |
| backend/app/seed/templates/snipeit-18mm.yaml | Adds Snipe-IT 18mm default template YAML. |
| backend/app/seed/templates/snipeit-24mm.yaml | Adds Snipe-IT 24mm default template YAML. |
| backend/app/seed/templates/spoolman-12mm.yaml | Adds Spoolman 12mm default template YAML. |
| backend/app/seed/templates/spoolman-18mm.yaml | Adds Spoolman 18mm default template YAML. |
| backend/app/seed/templates/spoolman-24mm.yaml | Adds Spoolman 24mm default template YAML. |
| backend/app/services/template_loader.py | Implements YAML template loading/caching and validation against IntegrationRegistry. |
| backend/pyproject.toml | Adds runtime pyyaml and dev types-PyYAML dependencies. |
| backend/tests/unit/schemas/test_template_schema.py | Updates schema tests for app: str | None and schema_version. |
| backend/tests/unit/seed/init.py | Initializes the tests.unit.seed package. |
| backend/tests/unit/seed/test_seed_templates.py | Adds smoke tests to ensure all shipped templates load and render. |
| backend/tests/unit/services/test_template_loader.py | Adds unit tests for TemplateLoader parsing, validation, caching, and reload behavior. |
Comments suppressed due to low confidence (1)
backend/app/services/template_loader.py:96
reload()clears_cachebefore attempting to re-load. If the directory contains an invalid/half-written YAML (likely during Phase-7 editor saves),load_dir()will raise and the cache will remain empty, effectively taking templates offline until a successful reload. Consider makingreload()transactional (load into a temporary cache, then swap on success) so a failed reload preserves the last known-good set.
@classmethod
def reload(cls, directory: Path) -> None:
"""Drop the cache then re-run load_dir.
Used by the future template editor (Phase 7) after a YAML write.
"""
cls._cache.clear()
cls.load_dir(directory)
| @classmethod | ||
| def load_dir(cls, directory: Path) -> None: | ||
| """Parse every ``*.yaml`` in ``directory`` and cache by template id. | ||
|
|
||
| Strict: any single-file failure raises TemplateValidationError | ||
| and the cache is left in whatever state it was before the call | ||
| (the broken file is not silently skipped — shipping a broken | ||
| seed template is a build-time bug). | ||
| """ | ||
| for path in sorted(directory.glob("*.yaml")): | ||
| template = cls._load_single(path) | ||
| cls._cache[template.id] = template | ||
|
|
| for path in sorted(directory.glob("*.yaml")): | ||
| template = cls._load_single(path) | ||
| cls._cache[template.id] = template | ||
|
|
|
All 3 Gemini review findings addressed in
Tests: 181 → 185 (+4 regression tests). Ruff check + format + mypy strict (on touched files) all clean. |
Phase 4 — Default Templates + TemplateLoader
Completes Phase 4 by shipping 12 default templates (Snipe-IT, Spoolman,
Grocy on 12/18/24mm, plus a generic QR-only set) and the loader that
validates them against the integration registry from Phase 3.5.
Schema changes
TemplateSchema.app:Literal[...]→str | None. The schema nolonger gates the plugin name; the loader validates against
IntegrationRegistryat load time. New integrations no longerrequire a schema migration.
app: nullmarks generic (QR-only)templates.
TemplateSchema.schema_version: newint = 1field. Hook forfuture YAML migrations.
TemplateLoader
load_dir(path): parses every*.yaml, strict-fail on the firstinvalid file. Non-YAML files (README, .gitkeep) are ignored.
reload(path): clears the cache and re-scans — used by thePhase-7 editor.
get(id),all(),by_app(app): lookup helpers.all()returns ashallow copy.
by_app(None)returns generic templates.TemplateValidationErrorraised on: YAML parse error, non-mappingroot, schema validation failure, unknown integration reference.
Templates
12 YAMLs in
backend/app/seed/templates/:All renderer pixel geometry confirmed: 12mm = 106px, 18mm = 165px,
24mm = 256px height at 600px canvas width.
App startup
main.pytriggersapp.integrationsimport (plugin discovery) thencalls
TemplateLoader.load_dir(SEED_DIR). After import,TemplateLoader.all()returns the 12 shipped templates.Tests
181 total (148 baseline + 4 schema + 15 loader + 13 seed + 1 from
PR #55's hardening commit). All pass.
schema_versiondefault + explicit,app: Nonefor generics, arbitrary plugin-name string accepted.
app: null, fourerror paths (non-mapping root, invalid YAML, missing fields,
unknown integration),
load_dirhappy path / strict-fail /ignore-non-YAML,
gethappy / missing-key,allshallow-copy,by_appfilter for known plugin and forNone,reloadcache-replacement and stale-entry-removal.
parametrized renders against dummy
LabelData.Test plan
pytest— 181/181 greenpytest --cov=app— 92.49% (above the 80% threshold)ruff check .— no errorsruff format --check .— cleanmypy app/schemas app/services app/seed app/integrations app/main.py tests/unit/seed tests/unit/services/test_template_loader.py tests/unit/schemas/test_template_schema.py --strict— cleanPRINTER_HUB_*env vars set):import app.main→registry has 3 plugins, loader has 12 templates, both
snipeit-24mmandqr-only-24mmrender to a 600x256 1-bitimage.
Known unrelated mypy debt
mypy --strictontests/unit/{schemas,services}surfaces 16 errorsin pre-existing test files unrelated to this branch
(
test_label_data.py,test_label_renderer.py,test_print_queue.py,and a few lines of
test_template_schema.pythat this branch did notmodify). All affected lines pre-date this branch. The new code paths
introduced here are mypy-clean; production code (
app/) isfully mypy-clean.
How to add a new template
<name>.yamlinbackend/app/seed/templates/with thelayout that fits your tape size and integration.
TemplateLoader.load_dirpicks it up at startup.idfield from the API/UI (Phase 6/7).For a new integration's templates, ensure the plugin is registered
in
backend/pyproject.tomlfirst (Phase 3.5); the loader willotherwise reject the template with
TemplateValidationError: unknown integration.Refs #22 — Phase 4 LabelRenderer + Templates is now complete.