Repository navigation
refactor(workflows): split the expression evaluator into a package - #5114
Merged
mnriem merged 3 commits intoOct 26, 2026
Merged
Conversation
added 3 commits
October 13, 2026 07:29
Move src/specify_cli/workflows/expressions.py (1571 lines) into a layered package so the evaluator, its shared leaves, the public API and the condition diagnostics are separately testable and a future engine can be swapped at a single point (engine.py). Behaviour is unchanged. Layout: - namespace/operators/filters/template: shared leaves, no internal deps - engine_current: the hand-written evaluator, interpolation and probe helpers - engine: the selection point, re-exporting the full engine contract - api: evaluate_expression / evaluate_condition - diagnostics: condition validation and remediation - __init__: logic-free facade keeping the historical public surface and the test-only private names importable The two drift-guard tests keep their assertions; only the monkeypatch target moves to the module that owns the global (namespace / engine_current). A new boundary test asserts that no module but engine.py and the facade imports an engine implementation (engine_*). Verified in the harness worktree against the S3 corpus: current backend 7/7 required, 78/78 compat expression, 11/11 compat condition. Assisted-by: opencode (model: deepseek-v4.1-flash, supervised)
Brings the package split up to date with main (79 commits). One conflict: main changed the monolith src/specify_cli/workflows/expressions.py, which this branch replaced with a package. Resolution: main's change to expressions.py (github#4923, short-circuit the right operand of and/or, with probes still visiting both operands) is ported into expressions/engine_current.py, and the monolith stays deleted. No other main-side change touched the evaluator. Every top-level definition of main's expressions.py exists in the package (46 of 46). Verified: uv sync --extra test, .venv/bin/python -m pytest -> 12757 passed, 435 skipped, 0 failed, including the probe/short-circuit and package-boundary tests. Assisted-by: Pi Coding Agent (model: deepseek-v4.1-flash, supervised)
The evaluator is no longer a single module, so both references in workflows/ARCHITECTURE.md named a file that no longer exists. Point the feature intro and the module tree at the package, naming the selection point (engine.py) and the public API module. Assisted-by: Pi Coding Agent (model: deepseek-v4.1-flash, supervised)
mnriem
added
the
triage-nice-to-have
Collaborator
|
Thank you! |
This was referenced Oct 26, 2026
kevinnguyenhoang91
pushed a commit
to kevinnguyenhoang91/spec-kit
that referenced
this pull request
Nov 10, 2026
…ithub#5114) * refactor: split workflow expression evaluator into a package Move src/specify_cli/workflows/expressions.py (1571 lines) into a layered package so the evaluator, its shared leaves, the public API and the condition diagnostics are separately testable and a future engine can be swapped at a single point (engine.py). Behaviour is unchanged. Layout: - namespace/operators/filters/template: shared leaves, no internal deps - engine_current: the hand-written evaluator, interpolation and probe helpers - engine: the selection point, re-exporting the full engine contract - api: evaluate_expression / evaluate_condition - diagnostics: condition validation and remediation - __init__: logic-free facade keeping the historical public surface and the test-only private names importable The two drift-guard tests keep their assertions; only the monkeypatch target moves to the module that owns the global (namespace / engine_current). A new boundary test asserts that no module but engine.py and the facade imports an engine implementation (engine_*). Verified in the harness worktree against the S3 corpus: current backend 7/7 required, 78/78 compat expression, 11/11 compat condition. Assisted-by: opencode (model: deepseek-v4.1-flash, supervised) * docs(workflows): point the architecture doc at the expression package The evaluator is no longer a single module, so both references in workflows/ARCHITECTURE.md named a file that no longer exists. Point the feature intro and the module tree at the package, naming the selection point (engine.py) and the public API module. Assisted-by: Pi Coding Agent (model: deepseek-v4.1-flash, supervised) --------- Co-authored-by: Markus <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Preparation for #5060, PR 1 of 3. Splits the 1581-line
src/specify_cli/workflows/expressions.pyinto a layered package and makesengine.pythe single place where an engine implementation is chosen. No behavior change.Layout
namespace.pyoperators.pyfilters.pytemplate.pyengine_current.pyengine.pyapi.pyevaluate_expression()/evaluate_condition()diagnostics.py__init__.pynamespace,operators,filtersandtemplateare leaves with no internal dependencies;engine_currentbuilds on them;engine,api,diagnosticsand the facade are the surfaces.The boundary this establishes
The point of the split is that a later engine can be swapped at one place.
tests/unit/test_expression_package_boundary.pypins that property in three assertions:engine.pyand the facade imports anengine_*implementation,engine.pyreally does import one (so the first test cannot pass vacuously),from .engine_current import ....The two existing drift guards keep their assertions; only the monkeypatch target moves to the module that now owns each global (
namespace.py,engine_current.py).Also in here
workflows/ARCHITECTURE.mdreferencedsrc/specify_cli/workflows/expressions.pyin two places — a file that no longer exists after this change. Both now point at the package, namingengine.pyandapi.py.Verification
uv sync --extra test && .venv/bin/python -m pytest→ 12757 passed, 435 skipped, 0 failedruff check src tests(the CI invocation) → cleanexpressions.py(46) exists in the packagemainup to date;main's fix(workflows): skip the right side of and/or when the left side decides #4923 short-circuit change is carried intoengine_current.py(probes still visit both operands)Not in this PR
PR 2 adds gap tests that pass on
main(including the three hand-written-parser bugs reported as #5071, #5072 and #5073). PR 3 replaces the parser with the Lark LALR grammar.AI Disclosure: PR body drafted with Pi Coding Agent (model: deepseek-v4.1-flash, reasoning effort high), under human supervision. The split itself was produced earlier with OpenCode (model: deepseek-v4.1-flash, supervised) and rebased, conflict-resolved and verified in this session with Pi Coding Agent (model: deepseek-v4.1-flash, supervised).