-
-
Notifications
You must be signed in to change notification settings - Fork 156
fix(776): allow alternate syntax for bash variables #786
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
base: master
Are you sure you want to change the base?
fix(776): allow alternate syntax for bash variables #786
Conversation
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.
👋 Thank you for your contribution 👍
I have left a comment. Please take a look at it 👀
Co-authored-by: Grachev Mikhail <work@mgrachev.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #786 +/- ##
==========================================
- Coverage 99.15% 99.11% -0.04%
==========================================
Files 46 48 +2
Lines 2834 3166 +332
==========================================
+ Hits 2810 3138 +328
- Misses 24 28 +4 ☔ View full report in Codecov by Sentry. |
Hi and thank you for the contribution! Just a nitpick: I would consider this as a |
("FOO=${NODE_ENV:development}", None), | ||
("BAR=${CI:+only}", None), | ||
("BUS=${var:=DEFAULT}", None), | ||
("FOOBAR=${var:?IS_UNSET}", None), |
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.
Btw, @mgrachev do you think it's ok just to allow all these chars?
#776 syntax still has some rules, while we may encounter something like: FOO=${SOME_VALUE--$$++???_END}
- which is invalid and moreover is buggy. It will substitute SOME_VALUE
and preserve remaining part
I don't think it's really good idea. Allowing alternative syntax, we also marks invalid ones as valid.
Proper solution would require some kind of parser here
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.
You're right, we definitely shouldn't skip such values: FOO=${SOME_VALUE--$$++???_END}
.
Perhaps we can check such values using a regular expression of the form \:[\+\=\?\-]?
?
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.
Regex may do the trick.
However, need to keep in mind that we also need to ensure it's only 1 time occurrence per substitution, and not something like:
${SOME_VALUE:+one:-two:?three}
It still possible ofc, but probably we can just go with simpler solution:
- pre-define the list of allowed sequences, e.g:
[":+", ":-", ":=", ":?"]
- ensure there is only one occurrence within the value
Also, need to ensure it will work with multiple substitutions correctly, e.g:
- positive test:
FOO=${NODE_ENV:development}${CI:+only}
- negative test: FOO=${NODE_ENV:development}${CI:+only:?IS_UNSET:other}`
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.
Good idea 👍
Hi @ro0NL 👋 First of all, requested changes must be applied: #786 (comment) Otherwise the PR will introduce another issue. @R11baka any chance you will take a look / work on this? Just to understand the state of PR Thanks everyone 🙏 |
✔ Checklist:
Description:
Fixes bug with alternate names. #776