From 722de697e98ea3ffbdcc192be66884b0f3a1a9e6 Mon Sep 17 00:00:00 2001 From: Ana Date: Mon, 12 Oct 2026 13:26:55 +0200 Subject: [PATCH 1/4] refactor: split module validation into structure checks and tree building No behaviour changes. The structure checks (required concepts, functional specs, implementation reqs) and the tree building (exports, acceptance tests, linked resources, concepts) become separate functions so a caller can run them at different points. --- src/plain_parser/plain_file.py | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/src/plain_parser/plain_file.py b/src/plain_parser/plain_file.py index 1aaadc9..de093cd 100644 --- a/src/plain_parser/plain_file.py +++ b/src/plain_parser/plain_file.py @@ -806,14 +806,8 @@ def process_exported_definitions(plain_source: dict, exported_definitions: list[ plain_source[plain_spec.DEFINITIONS].children.append(exported_definition) -def validate_and_marshall_module( - plain_file_parse_result: PlainFileParseResult, - module_name: str, - exported_definitions: list[mistletoe.block_token.token], - code_variables: dict, - template_dirs: list[str], -) -> dict: - """Every check a module must pass after parsing, and its marshalled plain source tree.""" +def validate_module_structure(plain_file_parse_result: PlainFileParseResult, module_name: str) -> None: + """Required concepts defined, functional specs present and non-empty, implementation reqs present.""" if len(plain_file_parse_result.required_concepts) > 0: missing_required_concepts_msg = "Missing required concepts: " missing_required_concepts_msg += ", ".join(plain_file_parse_result.required_concepts) @@ -841,6 +835,16 @@ def validate_and_marshall_module( has_requires=bool(plain_file_parse_result.required_modules), ) + +def validate_and_build_module_tree( + plain_file_parse_result: PlainFileParseResult, + module_name: str, + exported_definitions: list[mistletoe.block_token.token], + code_variables: dict, + template_dirs: list[str], +) -> dict: + """Merge exported definitions, validate acceptance tests, linked resources and concepts, and return the + marshalled plain source tree.""" process_exported_definitions(plain_file_parse_result.plain_source, exported_definitions) process_acceptance_tests(plain_file_parse_result.plain_source) @@ -863,6 +867,20 @@ def validate_and_marshall_module( return marshalled_plain_source +def validate_and_marshall_module( + plain_file_parse_result: PlainFileParseResult, + module_name: str, + exported_definitions: list[mistletoe.block_token.token], + code_variables: dict, + template_dirs: list[str], +) -> dict: + """Every check a module must pass after parsing, and its marshalled plain source tree.""" + validate_module_structure(plain_file_parse_result, module_name) + return validate_and_build_module_tree( + plain_file_parse_result, module_name, exported_definitions, code_variables, template_dirs + ) + + def _parse_module( plain_source_file_name: str, template_dirs: list[str] ) -> tuple[str, dict, list[str], list[tuple[str, dict]]]: From fbd74754c845e59e53cf19156a875197597e64bf Mon Sep 17 00:00:00 2001 From: Ana Date: Mon, 12 Oct 2026 13:57:07 +0200 Subject: [PATCH 2/4] fix: report a chain's errors top-down instead of deepest first Check the top module's structure before the requires walk, record required modules during the walk, then validate them top-down after the top module. This is the order codeplain reports errors in, so a broken chain now gives the same first error from `plain-parser check` and from codeplain's `--dry-run`. Each module is still read once. --- src/plain_parser/plain_file.py | 76 +++++++++++++++++++++++++--------- tests/test_requires.py | 69 +++++++++++++++++++++++++++++- 2 files changed, 125 insertions(+), 20 deletions(-) diff --git a/src/plain_parser/plain_file.py b/src/plain_parser/plain_file.py index de093cd..394b496 100644 --- a/src/plain_parser/plain_file.py +++ b/src/plain_parser/plain_file.py @@ -709,19 +709,31 @@ def parse_plain_file( ) +@dataclass +class RequiredModule: + """A required module as parsed by the requires walk; validated once the top module is complete.""" + + parse_result: PlainFileParseResult + # Definitions exported to this module by the modules it requires. + ancestor_exported_definitions: list[mistletoe.block_token.token] + code_variables: dict + # Definitions this module exports to the modules that require it. + exported_definitions: list[mistletoe.block_token.token] + + def process_required_modules( required_modules: list[str], template_dirs: list[str], all_required_modules: list[str], modules_trace: list[str], - chain: list[tuple[str, dict]], - module_exports: dict[str, list[mistletoe.block_token.token]], + walk_order: list[str], + required: dict[str, RequiredModule], ) -> list[mistletoe.block_token.token]: - """Parse and fully validate every required module, deepest first. + """Parse every required module, deepest first, without validating it. - Appends ``(module name, marshalled plain source tree)`` to ``chain`` for each module not already in it, - records each module's exported definitions in ``module_exports``, and returns the exported definitions - of the directly required modules. + Appends each module name to ``walk_order`` the first time it is reached (deepest ancestors first), + records its parse result in ``required``, and returns the exported definitions of the directly + required modules. """ exported_definitions = list[mistletoe.block_token.token]() for module_name in required_modules: @@ -730,7 +742,7 @@ def process_required_modules( if len(all_required_modules) > 0 and module_name == all_required_modules[-1]: # Already processed as an earlier entry; its exports are still visible to this module. - exported_definitions.extend(module_exports[module_name]) + exported_definitions.extend(required[module_name].exported_definitions) continue code_variables: dict = {} @@ -753,8 +765,8 @@ def process_required_modules( template_dirs, all_required_modules, modules_trace + [module_name], - chain, - module_exports, + walk_order, + required, ) own_exported_definitions = list[mistletoe.block_token.token]() @@ -770,20 +782,37 @@ def process_required_modules( exported_concept, plain_file_parse_result.plain_source, renderer ) ) - module_exports[module_name] = own_exported_definitions + required[module_name] = RequiredModule( + plain_file_parse_result, ancestor_exported_definitions, code_variables, own_exported_definitions + ) exported_definitions.extend(own_exported_definitions) - marshalled_plain_source = validate_and_marshall_module( - plain_file_parse_result, module_name, ancestor_exported_definitions, code_variables, template_dirs - ) - if module_name not in (chain_module_name for chain_module_name, _ in chain): - chain.append((module_name, marshalled_plain_source)) + if module_name not in walk_order: + walk_order.append(module_name) all_required_modules.append(module_name) return exported_definitions +def validate_required_modules( + required_modules: list[str], + required: dict[str, RequiredModule], + template_dirs: list[str], + trees: dict[str, dict], +) -> None: + """Validate recorded required modules top-down, in listed order, each module once.""" + for module_name in required_modules: + if module_name in trees: + continue + + module = required[module_name] + trees[module_name] = validate_and_marshall_module( + module.parse_result, module_name, module.ancestor_exported_definitions, module.code_variables, template_dirs + ) + validate_required_modules(module.parse_result.required_modules, required, template_dirs, trees) + + def process_exported_definitions(plain_source: dict, exported_definitions: list[mistletoe.block_token.token]) -> None: if len(exported_definitions) == 0: return @@ -908,20 +937,29 @@ def _parse_module( modules_trace=[], ) - chain: list[tuple[str, dict]] = [] + # Check the top module's structure, walk its requires, finish the top module's checks, then validate + # every required module top-down. + validate_module_structure(plain_file_parse_result, module_name) + + walk_order: list[str] = [] + required: dict[str, RequiredModule] = {} exported_definitions = process_required_modules( plain_file_parse_result.required_modules, template_dirs=template_dirs, all_required_modules=[], modules_trace=[], - chain=chain, - module_exports={}, + walk_order=walk_order, + required=required, ) - marshalled_plain_source = validate_and_marshall_module( + marshalled_plain_source = validate_and_build_module_tree( plain_file_parse_result, module_name, exported_definitions, code_variables, template_dirs ) + trees: dict[str, dict] = {} + validate_required_modules(plain_file_parse_result.required_modules, required, template_dirs, trees) + chain = [(chain_module_name, trees[chain_module_name]) for chain_module_name in walk_order] + return module_name, marshalled_plain_source, plain_file_parse_result.required_modules, chain diff --git a/tests/test_requires.py b/tests/test_requires.py index e375107..da2b8b1 100644 --- a/tests/test_requires.py +++ b/tests/test_requires.py @@ -1,7 +1,7 @@ import pytest from plain_parser import plain_file -from plain_parser.exceptions import ModuleDoesNotExistError, PlainSyntaxError +from plain_parser.exceptions import MissingFunctionalitiesError, ModuleDoesNotExistError, PlainSyntaxError def test_non_existent_require(get_test_data_path): @@ -154,6 +154,14 @@ def write_modules_using_required_exports(directory, requires_by_module): marks=pytest.mark.xfail(strict=True, reason="an ancestor must be listed before its dependent"), ), # Top + # || Top lists Base twice + # Base + pytest.param( + {"Top": ["Base", "Base"], "Base": []}, + ["Base", "Top"], + id="top-lists-base-twice", + ), + # Top # | # Upper every module also requires every module below it # | @@ -267,3 +275,62 @@ def test_requires_independent_branches_are_rejected(tmp_path, requires_by_module with pytest.raises(PlainSyntaxError, match="There must be a fixed order how required modules are dependent"): plain_file.parse_module_chain("Top.plain", [str(tmp_path)]) + + +# Two broken modules in one chain: each test pins which error is reported first. +def test_top_module_shape_is_checked_before_its_requires_are_read(tmp_path): + (tmp_path / "Top.plain").write_text("---\nrequires:\n - Missing\n---\n\n***implementation reqs***\n\n- Req.\n") + + with pytest.raises(MissingFunctionalitiesError): + plain_file.plain_file_parser("Top.plain", [str(tmp_path)]) + + +def test_top_module_shape_is_checked_before_required_modules_are_validated(tmp_path): + write_module(tmp_path, "Base", [], [], [], "Base uses :BaseMissing:.") + (tmp_path / "Top.plain").write_text("---\nrequires:\n - Base\n---\n\n***implementation reqs***\n\n- Req.\n") + + with pytest.raises(MissingFunctionalitiesError): + plain_file.plain_file_parser("Top.plain", [str(tmp_path)]) + + +def test_parse_errors_of_required_modules_come_before_top_module_concept_errors(tmp_path): + (tmp_path / "Base.plain").write_text( + "***implementation reqs***\n\n- {% if %}\n\n***functional specs***\n\n- Base works.\n" + ) + write_module(tmp_path, "Top", [], ["Base"], [], "Top uses :TopMissing:.") + + with pytest.raises(PlainSyntaxError, match="Invalid Liquid"): + plain_file.plain_file_parser("Top.plain", [str(tmp_path)]) + + +def test_top_module_concept_errors_come_before_required_module_concept_errors(tmp_path): + write_module(tmp_path, "Base", [], [], [], "Base uses :BaseMissing:.") + write_module(tmp_path, "Top", [], ["Base"], [], "Top uses :TopMissing:.") + + with pytest.raises(PlainSyntaxError, match="Concept :TopMissing: is not defined"): + plain_file.plain_file_parser("Top.plain", [str(tmp_path)]) + + +def test_required_modules_are_validated_top_down(tmp_path): + write_module(tmp_path, "Base", [], [], [], "Base uses :BaseMissing:.") + write_module(tmp_path, "Middle", [], ["Base"], [], "Middle uses :MiddleMissing:.") + write_module(tmp_path, "Top", [], ["Middle"], [], "Top works.") + + with pytest.raises(PlainSyntaxError, match="Concept :MiddleMissing: is not defined"): + plain_file.plain_file_parser("Top.plain", [str(tmp_path)]) + + +def test_module_listed_twice_around_its_dependent_is_rejected(tmp_path): + write_modules_using_required_exports(tmp_path, {"Top": ["Base", "Middle", "Base"], "Middle": ["Base"], "Base": []}) + + with pytest.raises(PlainSyntaxError, match="There must be a fixed order how required modules are dependent"): + plain_file.parse_module_chain("Top.plain", [str(tmp_path)]) + + +def test_chain_has_a_tree_for_every_module_reached_by_the_walk(tmp_path): + write_modules_using_required_exports(tmp_path, {"Top": ["Base", "Middle"], "Middle": ["Base"], "Base": []}) + + chain = plain_file.parse_module_chain("Top.plain", [str(tmp_path)]) + + assert [module_name for module_name, _ in chain] == ["Base", "Middle", "Top"] + assert all("functional specs" in tree for _, tree in chain) From 1b3451852eed15544df7cc8ac61123854f7dfaf5 Mon Sep 17 00:00:00 2001 From: Ana Date: Mon, 12 Oct 2026 16:44:54 +0200 Subject: [PATCH 3/4] test: pin required-module structure checks and single validation pass --- tests/test_requires.py | 42 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/tests/test_requires.py b/tests/test_requires.py index da2b8b1..0166b01 100644 --- a/tests/test_requires.py +++ b/tests/test_requires.py @@ -61,6 +61,7 @@ def test_plain_file_parser_validates_required_modules(get_test_data_path): def write_module(directory, module_name, exported_concepts, required_modules, definitions, functional_spec): + # functional_spec=None writes a module without a ***functional specs*** section. exports = "".join(f" - {concept}\n" for concept in exported_concepts) requires = "".join(f" - {required_module}\n" for required_module in required_modules) (directory / f"{module_name}.plain").write_text( @@ -70,8 +71,8 @@ def write_module(directory, module_name, exported_concepts, required_modules, de + "---\n\n" "***definitions***\n\n" + "".join(f"- {definition}\n\n" for definition in definitions) - + "***implementation reqs***\n\n- An implementation requirement.\n\n" - f"***functional specs***\n\n- {functional_spec}\n" + + "***implementation reqs***\n\n- An implementation requirement.\n" + + (f"\n***functional specs***\n\n- {functional_spec}\n" if functional_spec is not None else "") ) @@ -279,7 +280,7 @@ def test_requires_independent_branches_are_rejected(tmp_path, requires_by_module # Two broken modules in one chain: each test pins which error is reported first. def test_top_module_shape_is_checked_before_its_requires_are_read(tmp_path): - (tmp_path / "Top.plain").write_text("---\nrequires:\n - Missing\n---\n\n***implementation reqs***\n\n- Req.\n") + write_module(tmp_path, "Top", [], ["Missing"], [], None) with pytest.raises(MissingFunctionalitiesError): plain_file.plain_file_parser("Top.plain", [str(tmp_path)]) @@ -287,7 +288,7 @@ def test_top_module_shape_is_checked_before_its_requires_are_read(tmp_path): def test_top_module_shape_is_checked_before_required_modules_are_validated(tmp_path): write_module(tmp_path, "Base", [], [], [], "Base uses :BaseMissing:.") - (tmp_path / "Top.plain").write_text("---\nrequires:\n - Base\n---\n\n***implementation reqs***\n\n- Req.\n") + write_module(tmp_path, "Top", [], ["Base"], [], None) with pytest.raises(MissingFunctionalitiesError): plain_file.plain_file_parser("Top.plain", [str(tmp_path)]) @@ -334,3 +335,36 @@ def test_chain_has_a_tree_for_every_module_reached_by_the_walk(tmp_path): assert [module_name for module_name, _ in chain] == ["Base", "Middle", "Top"] assert all("functional specs" in tree for _, tree in chain) + + +def test_required_module_without_functional_specs_is_rejected(tmp_path): + write_module(tmp_path, "Base", [], [], [], None) + write_module(tmp_path, "Top", [], ["Base"], [], "Top works.") + + with pytest.raises(MissingFunctionalitiesError, match="Module 'Base'"): + plain_file.plain_file_parser("Top.plain", [str(tmp_path)]) + + +def test_top_module_concept_errors_come_before_required_module_structure_errors(tmp_path): + write_module(tmp_path, "Base", [], [], [], None) + write_module(tmp_path, "Top", [], ["Base"], [], "Top uses :TopMissing:.") + + with pytest.raises(PlainSyntaxError, match="Concept :TopMissing: is not defined"): + plain_file.plain_file_parser("Top.plain", [str(tmp_path)]) + + +def test_each_required_module_is_validated_once_top_down(tmp_path, monkeypatch): + write_modules_using_required_exports(tmp_path, {"Top": ["Base", "Middle"], "Middle": ["Base"], "Base": []}) + # A second validation pass over a module would be idempotent, so only the call count can observe it. + built = [] + build_tree = plain_file.validate_and_build_module_tree + + def record_and_build(parse_result, module_name, *args): + built.append(module_name) + return build_tree(parse_result, module_name, *args) + + monkeypatch.setattr(plain_file, "validate_and_build_module_tree", record_and_build) + + plain_file.parse_module_chain("Top.plain", [str(tmp_path)]) + + assert built == ["Top", "Base", "Middle"] From 485568447b679a7bd5fbb8e807d56469382f5537 Mon Sep 17 00:00:00 2001 From: Ana Date: Mon, 12 Oct 2026 16:40:18 +0200 Subject: [PATCH 4/4] refactor: store each required module's tree on its record --- src/plain_parser/plain_file.py | 48 +++++++++------------------------- 1 file changed, 13 insertions(+), 35 deletions(-) diff --git a/src/plain_parser/plain_file.py b/src/plain_parser/plain_file.py index 394b496..c4ce1c0 100644 --- a/src/plain_parser/plain_file.py +++ b/src/plain_parser/plain_file.py @@ -719,6 +719,8 @@ class RequiredModule: code_variables: dict # Definitions this module exports to the modules that require it. exported_definitions: list[mistletoe.block_token.token] + # Marshalled plain source tree, set once the module is validated. + tree: dict | None = None def process_required_modules( @@ -726,14 +728,12 @@ def process_required_modules( template_dirs: list[str], all_required_modules: list[str], modules_trace: list[str], - walk_order: list[str], required: dict[str, RequiredModule], ) -> list[mistletoe.block_token.token]: """Parse every required module, deepest first, without validating it. - Appends each module name to ``walk_order`` the first time it is reached (deepest ancestors first), - records its parse result in ``required``, and returns the exported definitions of the directly - required modules. + Records each module in ``required`` the first time it is reached (deepest ancestors first) and returns + the exported definitions of the directly required modules. """ exported_definitions = list[mistletoe.block_token.token]() for module_name in required_modules: @@ -765,7 +765,6 @@ def process_required_modules( template_dirs, all_required_modules, modules_trace + [module_name], - walk_order, required, ) @@ -787,30 +786,25 @@ def process_required_modules( ) exported_definitions.extend(own_exported_definitions) - if module_name not in walk_order: - walk_order.append(module_name) - all_required_modules.append(module_name) return exported_definitions def validate_required_modules( - required_modules: list[str], - required: dict[str, RequiredModule], - template_dirs: list[str], - trees: dict[str, dict], + required_modules: list[str], required: dict[str, RequiredModule], template_dirs: list[str] ) -> None: """Validate recorded required modules top-down, in listed order, each module once.""" for module_name in required_modules: - if module_name in trees: + module = required[module_name] + if module.tree is not None: continue - module = required[module_name] - trees[module_name] = validate_and_marshall_module( + validate_module_structure(module.parse_result, module_name) + module.tree = validate_and_build_module_tree( module.parse_result, module_name, module.ancestor_exported_definitions, module.code_variables, template_dirs ) - validate_required_modules(module.parse_result.required_modules, required, template_dirs, trees) + validate_required_modules(module.parse_result.required_modules, required, template_dirs) def process_exported_definitions(plain_source: dict, exported_definitions: list[mistletoe.block_token.token]) -> None: @@ -896,20 +890,6 @@ def validate_and_build_module_tree( return marshalled_plain_source -def validate_and_marshall_module( - plain_file_parse_result: PlainFileParseResult, - module_name: str, - exported_definitions: list[mistletoe.block_token.token], - code_variables: dict, - template_dirs: list[str], -) -> dict: - """Every check a module must pass after parsing, and its marshalled plain source tree.""" - validate_module_structure(plain_file_parse_result, module_name) - return validate_and_build_module_tree( - plain_file_parse_result, module_name, exported_definitions, code_variables, template_dirs - ) - - def _parse_module( plain_source_file_name: str, template_dirs: list[str] ) -> tuple[str, dict, list[str], list[tuple[str, dict]]]: @@ -941,14 +921,12 @@ def _parse_module( # every required module top-down. validate_module_structure(plain_file_parse_result, module_name) - walk_order: list[str] = [] required: dict[str, RequiredModule] = {} exported_definitions = process_required_modules( plain_file_parse_result.required_modules, template_dirs=template_dirs, all_required_modules=[], modules_trace=[], - walk_order=walk_order, required=required, ) @@ -956,9 +934,9 @@ def _parse_module( plain_file_parse_result, module_name, exported_definitions, code_variables, template_dirs ) - trees: dict[str, dict] = {} - validate_required_modules(plain_file_parse_result.required_modules, required, template_dirs, trees) - chain = [(chain_module_name, trees[chain_module_name]) for chain_module_name in walk_order] + validate_required_modules(plain_file_parse_result.required_modules, required, template_dirs) + # The walk records modules deepest first, which is the chain order. + chain = [(chain_module_name, module.tree) for chain_module_name, module in required.items()] return module_name, marshalled_plain_source, plain_file_parse_result.required_modules, chain