Conversation
This branch has not been deployed
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.
Closes #88.
stores.create_linesandstores.referenced_variableswere two functions that had to agreeabout what a create line expands, with nothing enforcing it. They are now one producer,
stores.create_steps, returning aCreateStep(line, variables)per store.The shape
Per store,
_createbinds the expandable text once and reads both outputs off thatbinding: the line through
to_shell, the variables throughvariable_names. Producing aline 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
orderandproject_diras separate arguments, so a caller could render lines against oneproject directory and report variables against another. One call closes that too.
Why the
environmentbranch changedIt used to hand-write
"${VAR-}"into the line and addVARto 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_shellalready owns ($VAR->${VAR-}, documented atshell._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:
environment,filewith${DIR},contentwith
${A},$$Band a bare$C, andfileplain) emits a byte-identical script andan identical
referenced_variableslist on this branch and onmain.environmentname grammar: of ~7200 candidates, the 96 that passstores._ENV_NAMEproduce 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) andjust test-conformance(907 passed) all green.
It does diverge for
environmentnamesstores.validaterefuses, in the safer direction:environment: 'VAR}; rm -rf /'used to render the unquoted"${VAR}; rm -rf /-}"and nowrenders the inert
"${VAR-}; rm -rf /}". Those documents never reach here --emit._plancalls
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
_createbeing source-compatible with what it replaced. For the samereason a non-string
environmentnow raisesTypeErrorrather than being stringified intothe line; unreachable past the gate, and no guard was added for a path no test can cover.
The new test
TestCreateStepPairingasserts the invariant the issue names -- for each step, the variablesthe 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
${NAMEwithoutaccounting for
to_shellescaping a literal dollar as\$, so a$${A}incontentrenders
\${A}and the naive scan read it as live. The fixture now carries$${D}and theoracle strips escapes first; reverting the oracle alone fails the test.
Not done here
f"{pod}-{kind.prefix}{name}"now appears three times instores.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 aproposal rather than folded in.