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

Escape $'s in reference default values #235

Merged
merged 2 commits into from
Jul 25, 2024
Merged

Conversation

huonw
Copy link
Contributor

@huonw huonw commented Jul 25, 2024

This escapes $ in default values to avoid them being treated as template literal substitutions.

Default values end up in a template literal `something`. If the string contains ${...} literally, they'll be treated as a substitution. This PR escapes all $s to avoid this.

This bug/oversight doesn't cause problems with any current values, but will be required in 2.23.0.dev4 due to the new default value for output_path fields from pantsbuild/pants#21175.

Example of the effect (from #234 (comment)):

- default_repr={`'${spec_path_normalized}/${target_name_normalized}${file_suffix}'`}
+ default_repr={`'\${spec_path_normalized}/\${target_name_normalized}\${file_suffix}'`}

default_repr={`$XDG_CACHE_HOME/pants/lmdb_store`}
default_repr={`\$XDG_CACHE_HOME/pants/lmdb_store`}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirming that this "spurious" escaping (i.e. not just immediately before a {, like ${...}), is still fine:
image

@huonw huonw requested review from tdyas, thejcannon and tgolsson July 25, 2024 06:25
Copy link
Contributor

@tdyas tdyas left a comment

Choose a reason for hiding this comment

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

Thanks for finding and fixing this. Definitely an unknown unknown for me.

@huonw
Copy link
Contributor Author

huonw commented Jul 25, 2024

Likewise! (just to confirm, this was an underlying problem in the doc processing here, nothing wrong with 21175!)

@huonw huonw merged commit efed535 into pantsbuild:main Jul 25, 2024
1 check passed
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.

2 participants