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

Allow variable substitution default value containing a colon #882

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

grthr
Copy link

@grthr grthr commented Aug 29, 2024

Allows to use a default value like this:

 "WEBSITE_URL": "${localEnv:WEBSITE_URL:https://example.com"

@grthr grthr marked this pull request as ready for review August 29, 2024 10:09
@grthr grthr requested a review from a team as a code owner August 29, 2024 10:09
Copy link
Contributor

@chrmarti chrmarti 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 the PR. Left a comment with my thoughts.

@@ -82,7 +82,7 @@ function evaluateSingleVariable(replace: Replace, match: string, variable: strin

// try to separate variable arguments from variable name
let args: string[] = [];
const parts = variable.split(':');
const parts = variable.split(':', 3);
Copy link
Contributor

Choose a reason for hiding this comment

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

This would prevent us from introducing additional arguments in the future because we would break existing configs.

Also: split() with the limit argument does not include the remaining part of the text after the limit was hit. This would need a closer look.

Copy link
Author

Choose a reason for hiding this comment

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

If we would need additional arguments in the future after the current ones it would be nice to be able to escape the : in the default value. Would this be an approach to go for?

something like "WEBSITE_URL": "${localEnv:WEBSITE_URL:https\://example.com"

@grthr grthr marked this pull request as draft September 11, 2024 07:28
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