Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix nested escaped dollarsigns #15

Merged
merged 2 commits into from
Sep 18, 2024
Merged

Fix nested escaped dollarsigns #15

merged 2 commits into from
Sep 18, 2024

Conversation

DrJosh9000
Copy link
Contributor

@DrJosh9000 DrJosh9000 commented Sep 17, 2024

What

Change how escaped expansions work back to only "expanding" to a $ - but still capture the potential identifier or brace expression that follows.

Given BAR=bar, $${FOO:$BAR} expands to ${FOO:bar} and $${FOO:$$BAR} expands to ${FOO:$BAR} (the pre-#10) behaviour).

While I'm here, do a bunch of work on the tests.

Why

Fixes #14

How

Originally, \$ and $$ were parsed into a single $ as plain text, and it didn't affect the interpretation of the following text.

We had the need to find potential "run-time" interpolations in buildkite-agent, so "escaped expansions" were added that parse like "normal" variable expansions, but produce different text when expanded. EscapedExpansion then had to be expanded (ahem) to handle brace expressions as well.

Here are the key changes in this PR:

 func (e EscapedExpansion) Expand(Env) (string, error) {
-       return "$" + e.Identifier, nil
+       return "$", nil
 }

and here.

Previously, EscapedExpansion would capture the identifier or brace expression that followed it. This caused problems for the interpretation of nested expansions: if the escaped dollarsign were treated simply as text as it should be, then expressions nested within the brace following the escaped dollarsign could be parsed and expanded as intended. Instead, because the brace was captured by EscapedExpansion, and expanded verbatim, that wouldn't happen.

Now, EscapedExpansion goes back to expanding only to $. Instead of capturing the following identifier or brace for expansion, it records what it was, then the parser restarts from the same position as text.

Copy link
Contributor

@moskyb moskyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is awesome - way more robust than my initial implementation! hopefully this clears up the issues we've been seeing.

Copy link
Contributor

@moskyb moskyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is awesome - way more robust than my initial implementation! hopefully this clears up the issues we've been seeing.

@moskyb
Copy link
Contributor

moskyb commented Sep 18, 2024

the approval so nice, i made it twice

@DrJosh9000 DrJosh9000 merged commit d30645a into main Sep 18, 2024
1 check passed
@DrJosh9000 DrJosh9000 deleted the making-it-rain branch September 18, 2024 00:14
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.

nested interpolations are broken
2 participants