feat: define XML preset schema (XSD v1.0) with formal validator and schema versioning - #63
Conversation
📊 PR SummaryChanges Overview
CI ChecksThis PR will trigger the following checks: Please ensure all checks pass before merging. |
… schema versioning Agent-Logs-Url: https://github.com/DirektDSP/PluginConfiguratorApp/sessions/074ed5a9-c7ad-4af8-9ac4-2386811da700 Co-authored-by: SeamusMullan <43112447+SeamusMullan@users.noreply.github.com>
📊 PR SummaryChanges Overview
CI ChecksThis PR will trigger the following checks:
Please ensure all checks pass before merging. |
|
@copilot fix failing type check lint and other jobs |
|
@copilot fix failing type check lint and other jobs |
Agent-Logs-Url: https://github.com/DirektDSP/PluginConfiguratorApp/sessions/0c773bcc-7b34-4069-8835-c050e87887d5 Co-authored-by: SeamusMullan <43112447+SeamusMullan@users.noreply.github.com>
Fixed in commit
|
📊 PR SummaryChanges Overview
CI ChecksThis PR will trigger the following checks:
Please ensure all checks pass before merging. |
There was a problem hiding this comment.
Pull request overview
Defines a formal XSD (v1.0) for preset XML files, introduces a Python XSD validator, and adds schema versioning metadata to preset round-tripping.
Changes:
- Add
preset_schema.xsdand a newPresetXSDValidator(lxml-backed) with unit tests. - Add
schema_version="1.0"support inConfigManagerand update bundled preset XMLs accordingly. - Document the schema/versioning and add
lxmlas a dependency.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Locks the new lxml dependency. |
| pyproject.toml | Adds lxml>=6.0.2 runtime dependency. |
| src/core/preset_validator.py | New XSD validator implementation using lxml. |
| src/core/config_manager.py | Adds SCHEMA_VERSION + reads/writes schema_version on preset root. |
| src/core/init.py | Exposes PresetXSDValidator from core. |
| src/resources/presets/preset_schema.xsd | New XSD v1.0 describing preset structure and types. |
| src/resources/presets/StandardAudioFX_Preset.xml | Adds root schema_version and new configuration fields. |
| src/resources/presets/MinimalPlugin_Preset.xml | Adds root schema_version and new configuration fields. |
| src/resources/presets/Instrument_Preset.xml | Adds root schema_version and new configuration fields. |
| docs/presets.md | Adds schema versioning + XSD documentation and field reference tables. |
| tests/test_preset_validator.py | Adds coverage for XSD validation behaviors and schema caching. |
| tests/test_config_manager.py | Updates sample config to include schema_version. |
| src/ui/dialogs/success_dialog.py | Refines callback typing for IDE buttons. |
| tests/test_success_dialog.py | Minor test cleanup (monkeypatch uses list .append directly). |
Comments suppressed due to low confidence (1)
src/ui/dialogs/success_dialog.py:245
btn.clicked.connect(callback)will pass thechecked: boolargument fromQPushButton.clicked(bool)intocallback. Since_detect_ides()returns callables that are intended to open a specific project path, ensure those callables won’t accidentally treat the emitted boolean as the path (which can lead tosubprocess.Popen([... , False])type errors). A common fix is to connect via a wrapper that ignores thecheckedparameter and calls the IDE function with no args (or define the IDE callbacks to accept and ignore*args).
@staticmethod
def _make_ide_button(label: str, callback: Callable[..., None]) -> QPushButton:
"""Return a styled IDE button that calls *callback* when clicked."""
icon_map = {"VSCode": "\U0001f4bb", "Xcode": "\U0001f528", "CLion": "\U0001f6e0"}
icon = icon_map.get(label, "\U0001f5a5")
btn = QPushButton(f"{icon} Open in {label}")
btn.setMinimumHeight(36)
btn.setToolTip(f"Open the project in {label}")
btn.clicked.connect(callback)
return btn
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SCHEMA_VERSION: ClassVar[str] = "1.0" | ||
| XSD_PATH: ClassVar[Path] = ( | ||
| Path(__file__).resolve().parents[1] / "resources" / "presets" / "preset_schema.xsd" | ||
| ) |
There was a problem hiding this comment.
PresetXSDValidator.SCHEMA_VERSION duplicates ConfigManager.SCHEMA_VERSION (added in this PR), which undermines the stated “single source of truth” for schema versioning and risks the constants drifting apart. Consider importing/deriving the validator’s version from ConfigManager.SCHEMA_VERSION (or moving the constant to a shared module) so only one value needs updating.
| @classmethod | ||
| def _get_schema(cls) -> etree.XMLSchema: | ||
| """Return the compiled XMLSchema, loading it on first access.""" | ||
| if cls._schema is None: | ||
| schema_doc = etree.parse(str(cls.XSD_PATH)) | ||
| cls._schema = etree.XMLSchema(schema_doc) | ||
| return cls._schema |
There was a problem hiding this comment.
_get_schema() can raise (e.g., missing/unreadable XSD file, invalid XSD, XMLSchemaParseError), which will escape from validate_file() / validate_string() despite those methods documenting a (is_valid, errors) return. Consider catching schema load/compile exceptions and returning (False, [...]) with a clear error message so callers don’t need to wrap every call in try/except.
| # schema_version is stored as an attribute on the root <preset> element, | ||
| # not as a child of <meta>. It is included here so that _apply_defaults | ||
| # preserves it in the meta dict when round-tripping config through save/load. | ||
| "schema_version": {"type": str, "default": "1.0"}, |
There was a problem hiding this comment.
META_FIELDS["schema_version"] hard-codes the default as "1.0" even though SCHEMA_VERSION was introduced just above. To keep schema versioning as a true single source of truth, set this default from SCHEMA_VERSION (and similarly avoid other duplicated literals) so bumps don’t require multiple edits.
| "schema_version": {"type": str, "default": "1.0"}, | |
| "schema_version": {"type": str, "default": SCHEMA_VERSION}, |
| config: dict[str, Any] = { | ||
| "meta": { | ||
| "name": root.attrib.get("name", file_path.stem), | ||
| # schema_version is stored as a root-element attribute, not as a | ||
| # child element of <meta>. Read it here before the META_FIELDS | ||
| # loop so the loop's "elif key not in config['meta']" guard will | ||
| # not overwrite it. | ||
| "schema_version": root.attrib.get("schema_version", self.SCHEMA_VERSION), | ||
| } |
There was a problem hiding this comment.
_load_structured_preset() now reads schema_version but never checks whether the value is supported/recognized. Since versioning is being introduced specifically to detect breaking format changes, consider rejecting (or explicitly warning/migrating) presets with an unknown schema_version rather than silently proceeding (which could load an incompatible preset incorrectly).
| - Two-layer validation is applied on every load and save: | ||
| 1. **XSD structural validation** via `PresetXSDValidator` (element names, nesting, types). | ||
| 2. **Semantic validation** via `ConfigManager.validate_config` (required fields, value | ||
| constraints). |
There was a problem hiding this comment.
The docs claim “Two-layer validation is applied on every load and save” via PresetXSDValidator, but the current code path (ConfigManager.load_config / _load_structured_preset / _save_structured_config) does not invoke PresetXSDValidator anywhere (and ConfigManager.validate_preset_file still only does semantic validation). Either wire XSD validation into the load/save/validate flow as described, or adjust this section so it doesn’t promise behavior the application doesn’t perform.
| - **Booleans**: `true`/`false` (case-insensitive; `1`/`0`/`yes`/`no`/`on`/`off` also | ||
| accepted when loading). |
There was a problem hiding this comment.
The boolean rules here say “case-insensitive”, but the XSD’s BooleanField ultimately relies on xs:boolean + lowercase enumerations (yes/no/on/off), which are case-sensitive. If XSD validation is intended to run (as described above), either update the docs to clarify that XSD validation requires lowercase boolean tokens, or expand the schema to accept the uppercase/mixed-case variants you want to support.
| - **Booleans**: `true`/`false` (case-insensitive; `1`/`0`/`yes`/`no`/`on`/`off` also | |
| accepted when loading). | |
| - **Booleans**: XSD validation accepts canonical boolean tokens: `true`/`false`, | |
| `1`/`0`, and lowercase `yes`/`no`/`on`/`off` where defined by the schema. Uppercase or | |
| mixed-case variants are not guaranteed to pass XSD validation when loading preset XML. |
Establishes a formal, machine-readable XML schema for preset files and wires up Python-side XSD validation — replacing the implicit structure defined only in
ConfigManager.PRESET_SCHEMA.Schema (
preset_schema.xsd)New W3C XSD at
src/resources/presets/preset_schema.xsd:project_info,configuration,implementations,user_experience,development_workflow) are required and ordered;<meta>is optionalBooleanFieldunion type acceptsxs:boolean(true/false/1/0) plus ConfigManager aliases (yes/no/on/off)xs:positiveInteger; all othersxs:string<preset>carries optionalnameandschema_versionattributesSchema versioning
ConfigManager.SCHEMA_VERSION = "1.0"added as a single source of truthschema_versionadded toMETA_FIELDS(default"1.0"); read from root attribute on load, written on saveschema_version="1.0"and the previously-missingconfigurationfields (au_component_type,au_component_subtype,au_component_manufacturer,au_version,clap_extensions,clap_features,auv3_platform)Validator (
PresetXSDValidator)Uses
lxml(new dependency, no known CVEs). The compiledXMLSchemais cached at the class level — parsed once per process.Docs
docs/presets.mdexpanded with: schema versioning behaviour, XSD reference + usage snippet, and a complete field-reference table for all five sections including types, defaults, and required-field annotations.