Skip to content

refactor: pair each store create line with the variables it expands - #137

Open
lesnik512 wants to merge 1 commit into
mainfrom
refactor/unify-store-create-seam
Open

lesnik512 wants to merge 1 commit into
mainfrom
refactor/unify-store-create-seam

Conversation

@lesnik512

Copy link
Copy Markdown
Member

Closes #88.

stores.create_lines and stores.referenced_variables were two functions that had to agree
about what a create line expands, with nothing enforcing it. They are now one producer,
stores.create_steps, returning a CreateStep(line, variables) per store.

The shape

Per store, _create binds the expandable text once and reads both outputs off that
binding: the line through to_shell, the variables through variable_names. Producing a
line without producing its variables is no longer expressible.

The old pair also let a caller disagree with itself in a second way: the two functions took
order and project_dir as separate arguments, so a caller could render lines against one
project directory and report variables against another. One call closes that too.

Why the environment branch changed

It used to hand-write "${VAR-}" into the line and add VAR to the variable set separately
-- the one branch where the two outputs came from different expressions, and a duplicate of
the unset-to-empty encoding shell.to_shell already owns ($VAR -> ${VAR-}, documented at
shell._encode_match). It now spells the reference ${VAR} and hands it to the same encoder,
so all three sources are one shape.

Behaviour

No change to anything the gate accepts. Evidence:

  • A document exercising all four source shapes (environment, file with ${DIR}, content
    with ${A}, $$B and a bare $C, and file plain) emits a byte-identical script and
    an identical referenced_variables list on this branch and on main.
  • Brute force over the environment name grammar: of ~7200 candidates, the 96 that pass
    stores._ENV_NAME produce identical lines and identical variables either way. ${NAME}
    takes to_shell's braced/no-operator arm, which emits exactly ${NAME-}.
  • just lint, just test-ci (100% coverage, 1472 passed) and just test-conformance
    (907 passed) all green.

It does diverge for environment names stores.validate refuses, in the safer direction:
environment: 'VAR}; rm -rf /' used to render the unquoted "${VAR}; rm -rf /-}" and now
renders the inert "${VAR-}; rm -rf /}". Those documents never reach here -- emit._plan
calls validate() itself (ADR-0004)
-- so this is not a fix being claimed, only a note that the no-op guarantee rests on that call
site rather than on _create being source-compatible with what it replaced. For the same
reason a non-string environment now raises TypeError rather than being stringified into
the line; unreachable past the gate, and no guard was added for a path no test can cover.

The new test

TestCreateStepPairing asserts the invariant the issue names -- for each step, the variables
the rendered line expands equal the variables that step reports. The oracle reads the
variables back off the emitted text rather than calling the production helper, so it is an
independent derivation.

Its first version was wrong, and the review caught it: it scanned for ${NAME without
accounting for to_shell escaping a literal dollar as \$, so a $${A} in content
renders \${A} and the naive scan read it as live. The fixture now carries $${D} and the
oracle strips escapes first; reverting the oracle alone fails the test.

Not done here

f"{pod}-{kind.prefix}{name}" now appears three times in stores.py (_flags_for,
teardown_line, create_steps). Two of those predate this change, and extracting
_store_name(pod, kind, name) would touch code #88 does not reach, so it is left as a
proposal rather than folded in.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unify the store render/vars seam in stores.py

1 participant