-
Notifications
You must be signed in to change notification settings - Fork 247
Conversation
Codecov Report
@@ Coverage Diff @@
## master #147 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 3 3
Lines 52 54 +2
Branches 11 11
=====================================
+ Hits 52 54 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for this! It looks good to me. Just a docs typo. I'd like another maintainer to review this as well!
README.md
Outdated
@@ -88,6 +88,8 @@ the parent. This is quite useful for launching the same command with different | |||
env variables or when the environment variables are too long to have everything | |||
in one line. | |||
|
|||
If you preceed a dollar sign with an odd number of backslashes the expression statement will not be replaced. Note that this means backslashes after the JSON string escaping took place. `"FOO=\\$BAR"` will not be replaced. `"FOO=\\\\$BAR"` will be replaced tough. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tough
Should be "though"
You're absolutely welcome. This tool saves my ass on a daily basis. Least thing I can do is to contribute ;) Fixed the typo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I'd still like another maintainer to give this a review.
return varNameWithDollarSign | ||
} | ||
return ( | ||
escapeChars.substr(0, escapeChars.length / 2) + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you walk me through why only half the back-slashes are required? I'm not sure I understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what happens in regular js string escaping too. \\
gets to be a \
. This is needed to allow somebody to write a \
in front of a $VARIABLE
and still replace the variable.
\$VAR
-> $VAR
\\$VAR
-> \value
\\\$VAR
-> \\$VAR
\\\\$VAR
-> \\value
and so on.
Hope this makes it clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Thank you. What about the other case? If the number of back-slashes is odd, isn't this code going to replace the expression by $VAR
every time? Shouldn't some back-slashes be retained?
Also I'm not sure your examples are quite consistent. In \$VAR
-> $VAR
, the back-slash is escaping the dollar sign. In \\$VAR
-> \value
the first back-slash escapes the second and the dollar sign is not escaped. Sounds fine up to there. Then in \\\$VAR
-> \\$VAR
, the first black-slash escapes the second, then the third escapes the dollar sign. So the result should be \$VAR
, shouldn't it? Maybe you made a typo? Fourth example looks right.
Any news? |
I think that I'm good with this. Thanks @sventschui 👍 |
Thank you very much! |
What:
Introduces the ability to escape dollar signs using a backslash (
"\\$"
). Using double backslash will escape the backslash ("\\\\$"
)e.g
Why:
I needed to have a dollar sign within my env var value
How:
Modifing the variable replacements regex
Checklist: